Perform topological sort to order components by cmacmackin · Pull Request #428 · boutproject/hermes-3

cmacmackin · 2025-11-27T18:24:54Z

Using the access control information introduced in #421, this PR makes it possible for Hermes-3 to work out the order of components at run-time. This will make things far simpler and more robust for users. It will also fail faster if there is an unsatisfiable or circular dependency.

Closes #384.

cmacmackin · 2025-11-27T18:30:22Z

This is nearly done. Remaining tasks:

Merge Control access to state variables in transform method #421 and rebase.
There is one integration test giving me strange h5py errors on my computer. It doesn't look to be related to any changes I've made. We'll see what happens in CI. I seem to recall we saw something similar elsewhere at one point.
Update documentation.

codecov · 2025-11-27T18:32:44Z

Codecov Report

❌ Patch coverage is 83.17308% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.26%. Comparing base (c69f8d3) to head (a5a9b0d).

Files with missing lines	Patch %	Lines
include/component.hxx	51.61%	11 Missing and 4 partials ⚠️
src/component.cxx	41.66%	7 Missing ⚠️
src/component_scheduler.cxx	95.80%	0 Missing and 6 partials ⚠️
src/electromagnetic.cxx	16.66%	5 Missing ⚠️
include/set_temperature.hxx	0.00%	1 Missing ⚠️
src/izn_rec_reaction.cxx	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #428      +/-   ##
==========================================
+ Coverage   59.73%   60.26%   +0.53%     
==========================================
  Files          98       98              
  Lines       10270    10420     +150     
  Branches     1482     1507      +25     
==========================================
+ Hits         6135     6280     +145     
- Misses       3488     3490       +2     
- Partials      647      650       +3

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The comments were in PR #445, but some of them actually relate to work done in #428. Only the latter are dealt with in this PR.

mikekryjak · 2026-01-07T13:48:51Z

I missed this in #421, but I see that you added a long-needed feature - a better way to determine species type which is based on the charge:

hermes-3/src/component_scheduler.cxx

Lines 339 to 352 in 8fc17e0

    
           // FIXME: Would there be any spcies without AA? Is there any other 
        
           // reliable way to identify what is a species? 
        
           else if (component_options[name_trimmed].isSet("AA")) { 
        
             if (component_options[name_trimmed].isSet("charge")) { 
        
               const BoutReal charge = component_options[name_trimmed]["charge"]; 
        
               if (charge > 1e-5) { 
        
                 positive_ions.push_back(name_trimmed); 
        
               } else if (charge < -1e-5) { 
        
                 negative_ions.push_back(name_trimmed); 
        
               } else { 
        
                 neutrals.push_back(name_trimmed); 
        
               } 
        
             } else { 
        
               neutrals.push_back(name_trimmed);

There is already a function that does the same in hermes_utils and is used in a few places in the code:

hermes-3/include/hermes_utils.hxx

Lines 61 to 70 in 053c9bb

    
           /// Identify species name string as electron, ion or neutral 
        
           inline SpeciesType identifySpeciesType(const std::string& species) { 
        
             if (species == "e") { 
        
               return SpeciesType::electron; 
        
             } else if ((species == "i") or 
        
                        species.find(std::string("+")) != std::string::npos) { 
        
               return SpeciesType::ion; 
        
             } 
        
             // Not electron or ion -> neutral 
        
             return SpeciesType::neutral;

Ideally there should be only one tool to do this, and your new one seems more robust. Is there any reason not to replace the one in hermes_utils with the new one?

cmacmackin · 2026-01-07T13:54:37Z

Ideally there should be only one tool to do this, and your new one seems more robust. Is there any reason not to replace the one in hermes_utils with the new one?

Probably not. I just wasn't sure if people would be happy about me changing that bit of code. Note that this will require changing the function signature of identifySpeciesType so that it takes the Options instead of just a string.

mikekryjak · 2026-01-07T13:55:01Z

I went through the sorting algorithm and I see that you have left a good amount of comments on the individual bits. However, I found it difficult to get my head around what's going on just because of the amount of steps involved. It would be very useful to have a paragraph describing how the algorithm works step-by-step, either in the docs or in the comments (or both)

cmacmackin · 2026-01-07T13:57:29Z

There are some places (e.g., when writing tests) where it was convenient to just be able to have a list of species names and use the old heuristics to categorise them. Probably not a good enough reason to keep that though.

ZedThree

LGTM, thanks @cmacmackin!

There's some trivial bits that I'm happy to fix myself

The comments were in PR #445, but some of them actually relate to work done in #428. Only the latter are dealt with in this PR.

Use CRTP to get component name for registration.

This required some changes to how options are accessed, so that we can more easily get/set only the domain or only the bounds.

bendudson · 2026-09-17T23:42:57Z

+  // Shouldn't change if called again
+  ASSERT_EQ(isSetFinalBoundary(option["test"]), false);
+}
+


GPT suggests a test to cover the GuardedOptions overload of IsSetFinalBoundary

Suggested change

/// isSetFinalBoundary should succeed without needing interior access,

/// lock boundary writes, and still allow interior writes.

TEST(ComponentTest, IsSetFinalBoundaryGuardedBoundaryOnly) {

Options options;

options["test"] = 1;

Permissions permissions({readOnly("test", Regions::Boundaries)});

GuardedOptions guarded(&options, &permissions);

ASSERT_TRUE(isSetFinalBoundary(guarded["test"]));

#if CHECKLEVEL >= 1

// Boundary is now assumed final

ASSERT_THROW(setBoundary<int>(options["test"], 2), BoutException);

// Interior/domain is still allowed

ASSERT_NO_THROW(setNoBoundary<int>(options["test"], 3));

ASSERT_EQ(getNoBoundary<int>(options["test"]), 3);

#endif

}

I think there are some other functions which should be tested like this too.

bendudson

Thanks @cmacmackin !

mikekryjak

I gave another serious go at going through everything and left a few comments. My preference would be that we merge this with a flag to disable the sort if needed (enabled by default is fine) as well as a facility to print the as-written and as-sorted component list to console on initialisation.

We also definitely need to make a release before merging this in.

mikekryjak · 2026-01-07T11:54:46Z

+/// permissions on the boundaries and read permissions for the
+/// interior.
+///
+/// FIXME: Currently these permissiosn are not expressed properly, due


is this FIXME still active? what does it take to fix it and what are the possible risks of leaving it unfixed?

mikekryjak · 2026-01-07T13:18:35Z

+  sorted.push_back(item);
+}
+
+/// Get all the parent sections of a variable "path". Sections are


Can you clarify that the path is just the hierarchy of the state object.

mikekryjak · 2026-01-07T13:19:37Z

+  std::set_intersection(unconditional_names.begin(), unconditional_names.end(),
+                        unconditional_sections.begin(), unconditional_sections.end(),
+                        std::inserter(sections_present, sections_present.begin()));
+  /// Assemble the set of all variable names which are definitlye


mikekryjak · 2026-09-18T16:40:44Z

-section defines the list of components used. The component order
-matters, as the components are executed in order.
+section defines the list of components used. The component order doesn't
+matters, as they will be sorted to ensure state variables are set


Suggested change

matters, as they will be sorted to ensure state variables are set

matter, as they will be sorted to ensure state variables are set

mikekryjak · 2026-09-18T16:41:53Z

+/// permissions on the boundaries and read permissions for the
+/// interior.
+///
+/// FIXME: Currently these permissiosn are not expressed properly, due


Suggested change

/// FIXME: Currently these permissiosn are not expressed properly, due

/// FIXME: Currently these permissions are not expressed properly, due

mikekryjak · 2026-09-18T17:14:33Z

+  for (const auto& component : components) {
+    const Permissions& permissions = component->getPermissions();
+    for (const auto& [varname, _] :
+         permissions.getVariablesWithPermission(PermissionTypes::ReadIfSet, true)) {


I found this section hard to understand because of the bool. Can you split it into getVariablesWithMinimumPermission and getVaraiblesWithPermission instead?

mikekryjak · 2026-09-18T17:20:53Z

+  std::set_intersection(unconditional_names.begin(), unconditional_names.end(),
+                        unconditional_sections.begin(), unconditional_sections.end(),
+                        std::inserter(sections_present, sections_present.begin()));
+  /// Assemble the set of all variable names which are definitly


Suggested change

/// Assemble the set of all variable names which are definitly

/// Assemble the set of all variable names which are definitely

mikekryjak · 2026-09-19T12:00:56Z

+    result[name] = {name};
+  }
+  // Sections map to those variables which they contain
+  for (const auto& section : sections_present) {


Claude flagged an issue here and I've spent quite a long time trying to figure out whether it's a bug or not. I've spent a couple of hours trying to untangle it but I'm still not 100% sure. I think this function would benefit from a clearer docstring at the top going through the algorithm more clearly and in a bit more detail. It's good you have comments at each of the step but I'm finding it tricky to put them all together in my head even though I know what the function is for.

I can see that you are making a tree of all the permissions. The complaint is that you are making the tree by initially looking at paths of unconditional_names only. Then you make sure to account for conditional_names as well, but you fill the map with blanks which are only filled if the right permissions exist in unconditional_names.

See a summary from Claude:

The hierarchy is built from `unconditional_names` only (L177-199). `conditional_names` get an entry at L206-208, but an empty one — nothing in this function fills those. They end up populated only if the same path independently arrives via the unconditional pile at L212/L220. So a `readIfSet` claim resolves correctly only when some component claims that exact same path with `readOnly` or stronger. Whenever reader and writer have to be matched through the tree, it fails — in both directions: Reader readIfSet("a:b") Writer readWrite("a:b:c") Reader readIfSet("a:b:c") Writer readWrite("a:b") In each case the `readIfSet` name keeps its empty entry, `expandVariableName` returns nothing, no dependency is created, and the reader may sort before the writer — silently. Swap the reader to `readOnly` and both order correctly, which is what makes this a bug rather than a limitation. Fix: run the parent scan and the section/variable split over `conditional_names ∪ unconditional_names`, then drop the L206-208 loop — every conditional name then gets its entry from one of the other two loops, which `expandVariableName`'s `.at()` requires.

I'll have a think about what the correct behaviour should be here. To be honest, I don't really like it if people give write permission for whole sections, because you can't actually guarantee they'll have written every variable in them that another component might use. I think the only reason I allowed permissions to be set for whole sections was so that components could read all the collision rates for a species without having to work out in advance exactly which ones were being computed.

I think this function would benefit from a clearer docstring at the top going through the algorithm more clearly and in a bit more detail.

Note that the overall algorithm is set out before the sortComponents function, as well as in the documentation.

mikekryjak · 2026-09-19T12:25:46Z

    component->declareAllSpecies(species);
  }
+
+  ::sortComponents(components);


We definitely need to have a flag to enable or disable the component sorting, as well as a console print at construction to print the input file component order and then the sorted list of components. I know you said this was in a separate PR - I'm assuming it's #445? How come it's not in this PR? It opens a risk of issues between this and the next one being merged.

I'm sure you've explained this to me before, apologies for not remembering all of this. It's definitely good to get this merged soon.

cmacmackin · 2026-09-21T17:48:49Z

+            {{"components", "a"},
+             {"a", {{"type", "orderchecker"}, {"permissions", toString(Permissions())}}}},
+            {"a"}),
+        // Single component with only read permissions


Suggested change

// Single component with only read permissions

// Single component with only write permissions

cmacmackin changed the title ~~WIP: Perform topologicalk sort to order components~~ WIP: Perform topological sort to order components Nov 27, 2025

cmacmackin marked this pull request as draft November 27, 2025 18:45

cmacmackin force-pushed the cmacmackin/topological_sort_components branch from a75b2ce to 8d049e2 Compare December 2, 2025 15:57

cmacmackin marked this pull request as ready for review December 5, 2025 15:07

cmacmackin changed the title ~~WIP: Perform topological sort to order components~~ Perform topological sort to order components Dec 5, 2025

This was referenced Dec 8, 2025

Control access to state variables in transform method #421

Merged

Remove use of const_cast with Options #443

Open

cmacmackin mentioned this pull request Dec 18, 2025

Allow components to call for creation of sub components #445

Open

cmacmackin added a commit that referenced this pull request Jan 2, 2026

Responded to some comments by @ZedThree

e650c1c

The comments were in PR #445, but some of them actually relate to work done in #428. Only the latter are dealt with in this PR.

cmacmackin force-pushed the cmacmackin/topological_sort_components branch from c31490e to e650c1c Compare January 2, 2026 14:28

cmacmackin added a commit that referenced this pull request Jan 5, 2026

Responded to some comments by @ZedThree

fff0581

The comments were in PR #445, but some of them actually relate to work done in #428. Only the latter are dealt with in this PR.

cmacmackin force-pushed the cmacmackin/topological_sort_components branch from e650c1c to 9dbeed5 Compare January 5, 2026 17:42

cmacmackin added a commit that referenced this pull request Jan 6, 2026

Responded to some comments by @ZedThree

b1dfb25

The comments were in PR #445, but some of them actually relate to work done in #428. Only the latter are dealt with in this PR.

cmacmackin force-pushed the cmacmackin/topological_sort_components branch from 9dbeed5 to 04a8977 Compare January 6, 2026 16:00

ZedThree pushed a commit that referenced this pull request Jan 7, 2026

Responded to some comments by @ZedThree

975a912

The comments were in PR #445, but some of them actually relate to work done in #428. Only the latter are dealt with in this PR.

ZedThree force-pushed the cmacmackin/topological_sort_components branch from 04a8977 to 8fc17e0 Compare January 7, 2026 10:46

ZedThree approved these changes Jan 7, 2026

View reviewed changes

Comment thread src/component_scheduler.cxx Outdated

Comment thread src/component_scheduler.cxx

Comment thread src/component_scheduler.cxx Outdated

Comment thread src/component_scheduler.cxx

cmacmackin commented Jan 7, 2026

View reviewed changes

Comment thread docs/sphinx/developer.rst Outdated

This was referenced Jan 14, 2026

Suppress permission warnings #468

Merged

Components not covered by integrated tests #475

Open

cmacmackin added a commit that referenced this pull request Jun 25, 2026

Responded to some comments by @ZedThree

73593b7

The comments were in PR #445, but some of them actually relate to work done in #428. Only the latter are dealt with in this PR.

cmacmackin force-pushed the cmacmackin/topological_sort_components branch from 8fc17e0 to 73593b7 Compare June 25, 2026 14:44

cmacmackin mentioned this pull request Jun 25, 2026

Recent changes change component order requirements for braginskii_conduction and sheath_boundary_simple #588

Closed

cmacmackin added a commit that referenced this pull request Jun 29, 2026

Responded to some comments by @ZedThree

70a94c1

The comments were in PR #445, but some of them actually relate to work done in #428. Only the latter are dealt with in this PR.

cmacmackin and others added 16 commits September 4, 2026 17:33

Mention new sorting and remove references to component ordering

5adc68b

Check results are the same regardless of component order

e0b2437

Check only one component can make final write

55afb99

Fixed some incorrect access controls causing errors

e4faa42

Add better comments explaining component sorting process

646573f

Responded to some comments by @ZedThree

654bd19

The comments were in PR #445, but some of them actually relate to work done in #428. Only the latter are dealt with in this PR.

Applied linter suggestions

3610219

Print meaningful error messages for circular dependencies

416468f

Addressed various comments on PR

bff5e68

external_apar: Use NamedComponent

2cdd49c

Use CRTP to get component name for registration.

Improve documentation/description for component sorting algorithm

58590d7

Fix permissions causing examples to fail

ffd2ec8

Fix permission issue

7d7a756

Ensure electromagnetic only sets interior for velocity/momentum

74e2e19

This required some changes to how options are accessed, so that we can more easily get/set only the domain or only the bounds.

Avoid debug 3D metrics build submitting empty reports to CodeCov

0ffde81

Fix test count expression

4572264

cmacmackin force-pushed the cmacmackin/topological_sort_components branch from aacebfa to 4572264 Compare September 4, 2026 16:33

bendudson reviewed Sep 17, 2026

View reviewed changes

Comment thread src/component.cxx Outdated

bendudson reviewed Sep 17, 2026

View reviewed changes

Comment thread docs/sphinx/developer.rst Outdated

bendudson reviewed Sep 17, 2026

View reviewed changes

Comment thread docs/sphinx/developer.rst Outdated

bendudson reviewed Sep 17, 2026

View reviewed changes

Comment thread docs/sphinx/developer.rst Outdated

bendudson reviewed Sep 18, 2026

View reviewed changes

Comment thread docs/sphinx/developer.rst Outdated

bendudson reviewed Sep 18, 2026

View reviewed changes

Comment thread include/permissions.hxx Outdated

bendudson and others added 3 commits September 18, 2026 08:35

Merge branch 'master' into cmacmackin/topological_sort_components

7ed72f0

Comment unit tests to explain what they are doing

10f8836

Apply suggestions from review

a5a9b0d

bendudson approved these changes Sep 18, 2026

View reviewed changes

mikekryjak requested changes Sep 19, 2026

View reviewed changes

cmacmackin commented Sep 21, 2026

View reviewed changes

+/// isSetFinalBoundary should succeed without needing interior access,
+/// lock boundary writes, and still allow interior writes.
+TEST(ComponentTest, IsSetFinalBoundaryGuardedBoundaryOnly) {
+  Options options;
+  options["test"] = 1;
+  Permissions permissions({readOnly("test", Regions::Boundaries)});
+  GuardedOptions guarded(&options, &permissions);
+  ASSERT_TRUE(isSetFinalBoundary(guarded["test"]));
+#if CHECKLEVEL >= 1
+  // Boundary is now assumed final
+  ASSERT_THROW(setBoundary<int>(options["test"], 2), BoutException);
+  // Interior/domain is still allowed
+  ASSERT_NO_THROW(setNoBoundary<int>(options["test"], 3));
+  ASSERT_EQ(getNoBoundary<int>(options["test"]), 3);
+#endif
+}

	matters, as they will be sorted to ensure state variables are set
	matter, as they will be sorted to ensure state variables are set

	/// FIXME: Currently these permissiosn are not expressed properly, due
	/// FIXME: Currently these permissions are not expressed properly, due

	/// Assemble the set of all variable names which are definitly
	/// Assemble the set of all variable names which are definitely

	// Single component with only read permissions
	// Single component with only write permissions

Conversation

cmacmackin commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmacmackin commented Nov 27, 2025 • edited by ZedThree Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mikekryjak commented Jan 7, 2026

Uh oh!

cmacmackin commented Jan 7, 2026

Uh oh!

mikekryjak commented Jan 7, 2026

Uh oh!

cmacmackin commented Jan 7, 2026

Uh oh!

ZedThree left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bendudson left a comment

Choose a reason for hiding this comment

Uh oh!

mikekryjak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cmacmackin commented Nov 27, 2025 •

edited

Loading

cmacmackin commented Nov 27, 2025 •

edited by ZedThree

Loading

codecov Bot commented Nov 27, 2025 •

edited

Loading