Skip to content

Perform topological sort to order components#428

Open
cmacmackin wants to merge 23 commits into
masterfrom
cmacmackin/topological_sort_components
Open

Perform topological sort to order components#428
cmacmackin wants to merge 23 commits into
masterfrom
cmacmackin/topological_sort_components

Conversation

@cmacmackin

@cmacmackin cmacmackin commented Nov 27, 2025

Copy link
Copy Markdown
Collaborator

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

cmacmackin commented Nov 27, 2025

Copy link
Copy Markdown
Collaborator Author

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.

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

codecov Bot commented Nov 27, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.97656% with 502 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.56%. Comparing base (d66b56c) to head (5f3578c).

Files with missing lines Patch % Lines
include/fixed_fraction_radiation.hxx 0.00% 157 Missing ⚠️
src/sheath_boundary_simple.cxx 20.53% 89 Missing ⚠️
include/detachment_controller.hxx 73.52% 27 Missing ⚠️
src/binormal_stpm.cxx 33.33% 20 Missing ⚠️
include/temperature_feedback.hxx 63.46% 19 Missing ⚠️
src/electromagnetic.cxx 38.70% 19 Missing ⚠️
include/simple_pump.hxx 0.00% 18 Missing ⚠️
include/upstream_density_feedback.hxx 14.28% 18 Missing ⚠️
src/temperature_feedback.cxx 0.00% 17 Missing ⚠️
src/anomalous_diffusion.cxx 39.13% 14 Missing ⚠️
... and 31 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #428      +/-   ##
==========================================
+ Coverage   49.09%   53.56%   +4.46%     
==========================================
  Files          96       97       +1     
  Lines       10037    10217     +180     
  Branches     1452     1485      +33     
==========================================
+ Hits         4928     5473     +545     
+ Misses       4604     4220     -384     
- Partials      505      524      +19     

☔ 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.

@cmacmackin cmacmackin marked this pull request as draft November 27, 2025 18:45
@cmacmackin cmacmackin force-pushed the cmacmackin/topological_sort_components branch from a75b2ce to 8d049e2 Compare December 2, 2025 15:57
@cmacmackin cmacmackin marked this pull request as ready for review December 5, 2025 15:07
@cmacmackin cmacmackin changed the title WIP: Perform topological sort to order components Perform topological sort to order components Dec 5, 2025
cmacmackin added a commit that referenced this pull request Jan 2, 2026
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 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
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 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
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 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
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 ZedThree force-pushed the cmacmackin/topological_sort_components branch from 04a8977 to 8fc17e0 Compare January 7, 2026 10:46
@mikekryjak

Copy link
Copy Markdown
Collaborator

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:

// 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:

/// 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

Copy link
Copy Markdown
Collaborator Author

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

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Collaborator Author

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 ZedThree left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @cmacmackin!

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

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
Comment thread docs/sphinx/developer.rst Outdated
cmacmackin added a commit that referenced this pull request Jun 25, 2026
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 cmacmackin force-pushed the cmacmackin/topological_sort_components branch from 8fc17e0 to 73593b7 Compare June 25, 2026 14:44
cmacmackin added a commit that referenced this pull request Jun 29, 2026
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 cmacmackin force-pushed the cmacmackin/topological_sort_components branch from ae0b58e to 533ec9f Compare June 29, 2026 10:31
bendudson and others added 15 commits June 29, 2026 16:49
Explicitly evaluate BinaryExpr expressions into fields when needed.
PetscLib throws if `type` exists in the `petsc` section.
This was created by iterating over all sections and checking
`options["type"].isValue()`. Instead use `options.isSet()`.
Contains:
- Fix for uninitialised dt in SNES
- PETSc preconditioner for CVODE
- BinaryExpr performance improvements
Note that components still aren't setting up any permissions, so there
would be runtime errors when executing the transform
methods. Furthermore, there are some missing methods I realise I need
for GuardedOptions. The unit tests are also failing to compile,
although I don't understand what the problem is for them, yet.
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 cmacmackin force-pushed the cmacmackin/topological_sort_components branch from 533ec9f to 14d5c62 Compare June 29, 2026 15:51
@cmacmackin

Copy link
Copy Markdown
Collaborator Author

I've merged in the changes from #596 as they are useful to writing error messages during the topological sort process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Automatically order components

4 participants