Perform topological sort to order components

[cmacmackin]

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]

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]

Codecov Report

❌ Patch coverage is 88.53047% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.52%. Comparing base (c69f8d3) to head (889c9d0).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
include/component.hxx	63.63%	8 Missing and 4 partials ⚠️
src/component_scheduler.cxx	94.53%	0 Missing and 10 partials ⚠️
src/electromagnetic.cxx	16.66%	5 Missing ⚠️
src/permissions.cxx	91.30%	1 Missing and 1 partial ⚠️
include/set_temperature.hxx	0.00%	1 Missing ⚠️
src/component.cxx	95.45%	0 Missing and 1 partial ⚠️
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.52%   +0.78%     
==========================================
  Files          98       98              
  Lines       10270    10479     +209     
  Branches     1482     1520      +38     
==========================================
+ Hits         6135     6342     +207     
+ Misses       3488     3481       -7     
- Partials      647      656       +9     

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

[mikekryjak]

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]

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]

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]

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

[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]

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

[cmacmackin]

This limitation still exists and it would be nontrivial to implement the desired feature. The permission system doesn't currently allow conditional logic that depends on any other variables or regions. I played around a bit with a system for providing this over last Christmas break, but it would significantly delay getting this merged if we wanted something like that.

I think the risks of leaving this unfixed are relatively low. It could, in principle, result in spurious errors from circular dependencies in the sorting algorithm, but nothing like that has surfaced so far. I'll cherrypick the commits allowing us to turn off the sorting algorithm and incorporate that into this PR, so we'd have a fallback if we do encounter a situation like this.

[cmacmackin]

I've addressed all comments left in review.