Allow components to call for creation of sub components - #445
cmacmackin wants to merge 7 commits into
Conversation
ZedThree
left a comment
ZedThree
left a comment
There was a problem hiding this comment.
I've tried to review the bits that I think have been modified for this feature.
My main comment is another architectural one, and we discussed this before: using a virtual method on Component means that we have to create a component before discovering if it creates any others. This basically takes those additional components out of the topological sort.
I guess in some ways this is YAGNI, but perhaps this is something we do need to consider?
| ComponentInformation(const std::string& name_, const std::string& type_) : name(name_), type(type_) {} | ||
|
|
||
| ComponentInformation(std::string&& name_, std::string&& type_) : name(std::move(name_)), type(std::move(type_)) {} | ||
|
|
There was a problem hiding this comment.
We should be able to rely on aggregate intialisation here, and so no need for constructors:
| ComponentInformation(const std::string& name_, const std::string& type_) : name(name_), type(type_) {} | |
| ComponentInformation(std::string&& name_, std::string&& type_) : name(std::move(name_)), type(std::move(type_)) {} |
| .withDefault<bool>(true); | ||
| } | ||
|
|
||
| virtual std::vector<ComponentInformation> additionalComponents() override { |
There was a problem hiding this comment.
override implies virtual, so we can drop the latter:
| virtual std::vector<ComponentInformation> additionalComponents() override { | |
| std::vector<ComponentInformation> additionalComponents() override { |
| std::string name; | ||
| std::string type; |
There was a problem hiding this comment.
Could use some docs or examples for these?
| // FIXME: Would there be any spcies without AA? Is there any other | ||
| // reliable way to identify what is a species? |
There was a problem hiding this comment.
I've seen this sort of thing in a few places, maybe there's some terrible magic we can wrought to store a Species struct of some kind in Options
There was a problem hiding this comment.
The problem is that the concept of species aren't really a first-class entity in Hermes-3. There isn't any fundamental concept of species in the input file syntax. There are just components, some of which will set variables for a particular species. There are many such components and a species can feature them in almost any combination. We could hypothetically have a list of all the components that can represent a species, but that risks becoming out-of-date as new components are written.
I suppose we could create a species sub-class of components, which those representing species should inherit from. I don't think there would be any way to enforce its use though.
There was a problem hiding this comment.
I think there are good reasons for not making species a first-class entity, as models may want to evolve things like groups of species (bundled charge states), different vibrational states, or energy groups (e.g. beam electrons), as distinct 'species', so what is meant by a species is intentionally flexible.
One possibility is to have the species 'self identify': Each species currently sets a 'charge' and 'AA' (mass) property in the state. We could just add a 'type' property that determines how it should be treated in e.g. collisions.
There was a problem hiding this comment.
I suppose another option would be to have an additional virtual method on components:
virtual std::optional<std::string> species() const { return {}; }
If the component evolves a species then it would override the method to return that species' name (which is usually the same as the component name).
There was a problem hiding this comment.
We could then have a sub-class
struct SpeciesComponent : public Component {
SpeciesComponent(std::string name, Permissions access_permissions) :
Component(access_permissions), name(std::move(name)) {}
std::optional<std::string> species() const override { return name; }
private:
std::string name;
}
All the components corresponding to species (e.g., EvolveDensity, EvolveMomentum, Vorticity, etc.) could then inherit from that, so they don't need to bother reimplementing the method.
| std::set<ComponentInformation> created_components; | ||
| for (auto it = required_components.begin(); it != required_components.end(); | ||
| it = required_components.begin()) { | ||
| auto comp = Component::create(it->type, it->name, component_options, solver); | ||
| for (const auto& sub_comp : comp->additionalComponents()) { | ||
| if (required_components.count(sub_comp) == 0 | ||
| and created_components.count(sub_comp) == 0) { | ||
| required_components.insert(sub_comp); | ||
| } | ||
| } | ||
| components.push_back(std::move(comp)); | ||
| created_components.insert(required_components.extract(it)); | ||
| } |
There was a problem hiding this comment.
This bit could use some commenting!
This shouldn't take the new components out of the topological sort. The sorting only happens after all components have been created, so adding more components won't affect that. |
00a27c5 to
1d0d823
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## cmacmackin/cache_bout_builds #445 +/- ##
================================================================
+ Coverage 60.09% 60.24% +0.15%
================================================================
Files 97 98 +1
Lines 10254 10313 +59
Branches 1488 1496 +8
================================================================
+ Hits 6162 6213 +51
- Misses 3465 3469 +4
- Partials 627 631 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1d0d823 to
736052b
Compare
|
The I've tried rewriting it slightly so that it will go back to using the order in the input file as its starting point for topological sorting. This makes the test pass, but I'm still concerned by what I'm seeing. It would appear that the permission information I am currently specifying on some components is not adequate to guarantee the same result. Perhaps something hasn't been marked as the final-write that should be. I note that this is the only integration test which has had this problem; all the others worked fine even with a radical reordering of the components. That might give us something to go on. |
|
Here is the order the components are executed when I get the error described above:
@mikekryjak, @bendudson is there anything in this ordering that stands out to you as wrong? |
|
Hi @cmacmackin ! |
|
@bendudson Edit: This is for the integration test |
|
I've added the ability to turn the sorting feature on and off using the |
|
I've worked out the two components that are causing problems (although not why): |
|
Rebasing this PR seems to have fixed some of the ordering problems (presumably due to various fixes we've made to permissions in the past months). However, I've found that you can still get issues depending on whether you put |
d6cabb9 to
eaac8fe
Compare
eaac8fe to
84f53b4
Compare
3 participants
This PR adds a virtual method to Component classes which lists the names and types of additional components which they might need. This information is used by the ComponentScheduler class to create these additional components automatically. Users can override default values for the configurations of these additional components by adding a named section to their config file, as usual. A
BraginskiiClosurecomponent has been added, which does nothing except call for the creation of all the other Braginskii components.Closes #386