Skip to content

Conversation

@cmacmackin
Copy link
Collaborator

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 BraginskiiClosure component has been added, which does nothing except call for the creation of all the other Braginskii components.

Closes #386

@cmacmackin cmacmackin added the enhancement New feature or request label Dec 15, 2025
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.
Also need to revert a bunch of changes from
Options::isSet(std::string) to IS_SET, to avoid creating objects we
don't need.
All unit and integration tests pass, but many components are
untested. There may be errors in their permissions.
Also added an additional constructor for SpeciesInfo
@cmacmackin
Copy link
Collaborator Author

@ZedThree A lot of these comments actually apply to #428. I should have mentioned in my opening comment that we should wait to review this PR until after that one has been merged.

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 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/create_sub_components branch from 00a27c5 to 1d0d823 Compare January 5, 2026 18:17
@codecov
Copy link

codecov bot commented Jan 5, 2026

Codecov Report

❌ Patch coverage is 66.69361% with 412 lines in your changes missing coverage. Please review.
✅ Project coverage is 30.70%. Comparing base (e0f1ebf) to head (736052b).
⚠️ Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
src/vorticity.cxx 0.00% 28 Missing ⚠️
src/polarisation_drift.cxx 0.00% 21 Missing ⚠️
src/classical_diffusion.cxx 0.00% 18 Missing ⚠️
src/sheath_boundary_simple.cxx 0.00% 16 Missing ⚠️
include/detachment_controller.hxx 0.00% 15 Missing ⚠️
src/component_scheduler.cxx 90.00% 9 Missing and 6 partials ⚠️
include/component.hxx 81.08% 10 Missing and 4 partials ⚠️
include/fixed_temperature.hxx 0.00% 13 Missing ⚠️
include/set_temperature.hxx 0.00% 13 Missing ⚠️
src/sheath_boundary_insulating.cxx 0.00% 12 Missing ⚠️
... and 52 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #445      +/-   ##
==========================================
+ Coverage   26.41%   30.70%   +4.28%     
==========================================
  Files          90       95       +5     
  Lines        8105     8934     +829     
  Branches     1133     1262     +129     
==========================================
+ Hits         2141     2743     +602     
- Misses       5745     5917     +172     
- Partials      219      274      +55     

☔ View full report in Codecov by Sentry.
📢 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.

- Unused arguments in some of the functions for GuardedOptions
- Unit tests expecting an exception to be thrown, but this only
  happens when checking is done.
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/create_sub_components branch from 1d0d823 to 736052b Compare January 6, 2026 16:07
@cmacmackin
Copy link
Collaborator Author

The 1D-recycling-dthe integration test is failing in CI, with relative differences on the order of >1e-2. (Far too high!) This appears to be due to the radically different order in which components end up being built, which is a side-effect of some of the changes in the PR. Previously, the sorting algorithm used the order given in the input file as its starting point. Now it basically sorts the components alphabetically by name and type before it does the topological sort. This means that the final order of the components after topological sorting is now radically different from what is given in the input file.

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.

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.
@cmacmackin
Copy link
Collaborator Author

Here is the order the components are executed when I get the error described above:

  • d (evolve_density)
  • d (evolve_pressure)
  • d+ (evolve_density)
  • d+ (evolve_pressure)
  • he (evolve_density)
  • he+ (evolve_density)
  • t (evolve_density)
  • t+ (evolve_density)
  • e (quasineutral)
  • e (evolve_pressure)
  • he (evolve_pressure)
  • he+ (evolve_pressure)
  • t (evolve_pressure)
  • t+ (evolve_pressure)
  • braginskii_collisions (braginskii_collisions)
  • d (evolve_momentum)
  • d (noflow_boundary)
  • he (evolve_momentum)
  • he (noflow_boundary)
  • d+ (evolve_momentum)
  • d+ (noflow_boundary)
  • he+ (evolve_momentum)
  • t (evolve_momentum)
  • t+ (evolve_momentum)
  • e (zero_current)
  • e (noflow_boundary)
  • he+ (noflow_boundary)
  • t+ (noflow_boundary)
  • sheath_boundary (sheath_boundary)
  • reactions (d + d+ -> d+ + d)
  • t (noflow_boundary)
  • reactions (d + t+ -> d+ + t)
  • reactions (t + d+ -> t+ + d)
  • reactions (t + t+ -> t+ + t)
  • braginskii_conduction (braginskii_conduction)
  • braginskii_ion_viscosity (braginskii_ion_viscosity)
  • braginskii_thermal_force (braginskii_thermal_force)
  • d+ (upstream_density_feedback)
  • reactions (d + e -> d+ + 2e)
  • reactions (d+ + e -> d)
  • reactions (he + e -> he+ + 2e)
  • reactions (he+ + e -> he)
  • reactions (t + e -> t+ + 2e)
  • reactions (t+ + e -> t)
  • electron_force_balance (electron_force_balance)
  • he+ (upstream_density_feedback)
  • neutral_parallel_diffusion (neutral_parallel_diffusion)
  • recycling (recycling)
  • t+ (upstream_density_feedback)

@mikekryjak, @bendudson is there anything in this ordering that stands out to you as wrong?

@bendudson
Copy link
Collaborator

Hi @cmacmackin !
This ordering looks cromulent to me. The only thing I can see that's different is t (noflow_boundary) comes after sheath_boundary (sheath_boundary). That shouldn't make a difference because sheath_boundary should ignore t because it's a neutral species.

@cmacmackin
Copy link
Collaborator Author

cmacmackin commented Jan 7, 2026

@bendudson
Thanks for taking a look. The fact that this ordering is giving badly wrong answers for some reaction stuff (and takes significantly longer to run) means that something is wrong. It would appear that there might be some implicit ordering or dependency that we've all missed. Here is the error message in case that helps us narrow things down:

Ed+t_cx differs from reference data by > 20000000000000.0 ULPs
 Value = -0.0004365854362020518
 Reference = -0.00044916942235749286
Edd+_cx differs from reference data by > 20000000000000.0 ULPs
 Value = 0.00015620791788336535
 Reference = 0.0001584462055586012
Edt+_cx differs from reference data by > 20000000000000.0 ULPs
 Value = -0.0004107942463354931
 Reference = -0.0004246825908348509
Et+d_cx differs from reference data by > 20000000000000.0 ULPs
 Value = -0.0006942134061277697
 Reference = -0.000703213458219236
Etd+_cx differs from reference data by > 20000000000000.0 ULPs
 Value = -0.000541220600109632
 Reference = -0.0005438529063749786
Ett+_cx differs from reference data by > 20000000000000.0 ULPs
 Value = 3.918542691027873e-05
 Reference = 4.1129091954560007e-05
Fd+t_cx differs from reference data by > 20000000000000.0 ULPs
 Value = -0.00011951942056416081
 Reference = -0.00012159622040998301
Fdd+_cx differs from reference data by > 20000000000000.0 ULPs
 Value = 0.00023478567579766294
 Reference = 0.00023601241685116078
Fdt+_cx differs from reference data by > 20000000000000.0 ULPs
 Value = -1.4960423611226284e-05
 Reference = -1.480474061534612e-05
Ft+d_cx differs from reference data by > 20000000000000.0 ULPs
 Value = -7.903779726718753e-05
 Reference = -8.170568455767375e-05
Ftd+_cx differs from reference data by > 20000000000000.0 ULPs
 Value = 4.858695424123787e-05
 Reference = 4.734980178143874e-05
Ftt+_cx differs from reference data by > 20000000000000.0 ULPs
 Value = 6.0414168626410105e-05
 Reference = 6.209672003436238e-05
Kdd+_cx differs from reference data by > 20000000000000.0 ULPs
 Value = 0.01075641223571999
 Reference = 0.010612777344029932
Kdt+_cx differs from reference data by > 20000000000000.0 ULPs
 Value = 0.0054615181858046835
 Reference = 0.005504396701388891
Ktd+_cx differs from reference data by > 20000000000000.0 ULPs
 Value = 0.010736211325369564
 Reference = 0.010560574694497988
Ktt+_cx differs from reference data by > 20000000000000.0 ULPs
 Value = 0.005450886137841182
 Reference = 0.005475965585803034
Sdt+_cx differs from reference data by > 20000000000000.0 ULPs
 Value = -0.0007744787669173392
 Reference = -0.0007849565120605158
Std+_cx differs from reference data by > 20000000000000.0 ULPs
 Value = -0.0006899011832115692
 Reference = -0.0006956104988655306

Edit: This is for the integration test 1D-recycling-dthe.

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow components to create new components for themselves

4 participants