Improve functions to retrieve fields of certain type from introspection #5643
Conversation
gassmoeller
force-pushed
the
function_to_retrieve_fields_of_type
branch
from
d213f3f
to
ee12034
Compare
| enum Type | ||
| { | ||
| chemical_composition, | ||
| stress, | ||
| strain, | ||
| grain_size, | ||
| porosity, | ||
| density, | ||
| entropy, | ||
| generic, | ||
| unspecified | ||
| chemical_composition = 0, | ||
| stress = 1, | ||
| strain = 2, | ||
| grain_size = 3, | ||
| porosity = 4, | ||
| density = 5, | ||
| entropy = 6, | ||
| generic = 7, | ||
| unspecified = 8 | ||
| } type; |
There was a problem hiding this comment.
Do you actually make use of these numeric values? My preference is to treat enums as symbolic names independent of any numerical representation.
There was a problem hiding this comment.
Yes, I use these values to index into an std::vector below. That is why I explicitly specified them to make sure I can uniquely do the indexing. If I would just treat the enums as symbolic names I would create a utility function with a switch statement to return an unsigned int depending on type. However, I think according to the standard I am actually allowed to implicitly cast the enums to the unsigned ints.
There was a problem hiding this comment.
I just dislike this from a conceptual perspective. It requires keeping track of things in different places.
What would you think of keeping the enum as just a list of symbolic names, and storing data instead of a fixed-size vector into a std::map<enum_type, value_type>?
There was a problem hiding this comment.
I am not happy about that solution because I do not know the performance implications. The cost of a lookup into a std::map already suprised us in the particle code before and the reason for this PR was to allow this lookup to be fast enough to be done multiple times per cell. What do you think about the alternative of keeping the enum as just a list of names, and having a helper function in an anonymous namespace in introspection.cc that is essentially:
unsigned int
index_of_type (enum type)
{
switch (type):
case type_a:
return 0;
case type_b:
return 1;
...
}
This way no code outside of Introspection can see that we translate the enum values into indices of a std::vector. However, it requires to always change the function if we change the enum. The beaty of the current solution is that no other code is affected if we add another enum option, everything will just work like before.
There was a problem hiding this comment.
But you still have to have two places that you keep in sync: The translation (whether in a function or by providing initializers) and the place where you store the number of enum elements.
I can be ok with the current state. Let's not make it more difficult!
| const std::vector<unsigned int> | ||
| const std::vector<unsigned int> & | ||
| get_indices_for_fields_of_type (const CompositionalFieldDescription::Type &type) const; | ||
|
|
||
| /** | ||
| * Get the names of the compositional fields which are of a | ||
| * particular type (chemical composition, porosity, etc.). | ||
| */ | ||
| const std::vector<std::string> | ||
| const std::vector<std::string> & |
There was a problem hiding this comment.
That was a bug to begin with. The fact that we marked these return types as const suggests that we always intended for them to be references.
|
I added the assert's you asked for. |
source/simulator/introspection.cc
Outdated
| Introspection<dim>::composition_type_exists (const CompositionalFieldDescription::Type &type) const | ||
| { | ||
| for (unsigned int c=0; c<composition_descriptions.size(); ++c) | ||
| if (composition_descriptions[c].type == type) | ||
| return true; | ||
| return false; | ||
| return composition_indices_for_type[type].size() > 0; |
There was a problem hiding this comment.
Just add an assert here that type is a valid index into the array. This way we'll catch the case where we add an enum element but forget to update n_types. Same in the other places where you use type as an index.
| if (composition_indices_for_type[type].size() > 0) | ||
| return composition_indices_for_type[type][0]; |
gassmoeller
force-pushed
the
function_to_retrieve_fields_of_type
branch
from
a1de925
to
dc9f2d4
Compare
gassmoeller
force-pushed
the
function_to_retrieve_fields_of_type
branch
from
dc9f2d4
to
fe04d81
Compare
|
Ah, sorry I missed those two functions. Fixed now, and I now compare against the actual size of the array, not the |
|
Great, thank you! |
This PR improves the functionality inside the
Introspectionclass to retrieve the names or indices of certain field types. In particular it changes:push_backinto a vector which is then returned by-value)This PR should allow to retrieve this information quickly even in loops over cells without significant performance penalty.
This should help somewhat with an application in #4370.
@anne-glerum, @jdannberg or @bobmyhill could you take a look at this PR?