Update detection of skipped/undeclared species

[ischoegl]

Changes proposed in this pull request

• Fix skipping of undeclared species with specific third body detection
• Streamline detection of undeclared species in other cases
• Output all missing species, not just first
• Update formatting for Reaction::validate

While IonBurnerFlame does act up, this is not the first time. I have skipped those tests for the time being.

If applicable, fill in the issue number this pull request is fixing

Fixes #1024

E.g. if skip-undeclared-third-bodies=false, the error is formatted as

*******************************************************************************
InputFileError thrown by Reaction::checkSpecies:
Error on line 215 of /src/build/python/cantera/data/h2o2.yaml:
Reaction 'H + O2 + N2 <=> HO2 + N2'
is a three-body reaction with undeclared species: 'N2'
|  Line |
|   210 |   efficiencies: {O2: 0.0, H2O: 0.0, N2: 0.0, AR: 0.0}
|   211 | - equation: H + 2 O2 <=> HO2 + O2  # Reaction 7
|   212 |   rate-constant: {A: 2.08e+19, b: -1.24, Ea: 0.0}
|   213 | - equation: H + O2 + H2O <=> HO2 + H2O  # Reaction 8
|   214 |   rate-constant: {A: 1.126e+19, b: -0.76, Ea: 0.0}
>   215 > - equation: H + O2 + N2 <=> HO2 + N2  # Reaction 9
            ^
|   216 |   rate-constant: {A: 2.6e+19, b: -1.24, Ea: 0.0}
|   217 | - equation: H + O2 + AR <=> HO2 + AR  # Reaction 10
|   218 |   rate-constant: {A: 7.0e+17, b: -0.8, Ea: 0.0}
*******************************************************************************

Checklist

• There is a clear use-case for this code change
• The commit message has a short title & references relevant issues
• Build passes (scons build & scons test) and unit tests address code coverage
• Style & formatting of contributed code follows contributing guidelines
• The pull request is ready for review

[codecov]

Codecov Report

Merging #1027 (5c83534) into main (19f1593) will increase coverage by 0.01%.
The diff coverage is 96.51%.

❗ Current head 5c83534 differs from pull request most recent head eee271c. Consider uploading reports for the commit eee271c to get more accurate results

@@            Coverage Diff             @@
##             main    #1027      +/-   ##
==========================================
+ Coverage   72.54%   72.55%   +0.01%     
==========================================
  Files         355      355              
  Lines       46422    46446      +24     
==========================================
+ Hits        33675    33701      +26     
+ Misses      12747    12745       -2     

Impacted Files	Coverage Δ
include/cantera/kinetics/Kinetics.h	55.88% <ø> (+4.36%)	⬆️
include/cantera/kinetics/Reaction.h	100.00% <ø> (ø)
src/base/ctexceptions.cpp	85.00% <ø> (ø)
src/kinetics/GasKinetics.cpp	97.35% <ø> (-0.09%)	⬇️
src/kinetics/Kinetics.cpp	84.12% <50.00%> (-2.35%)	⬇️
src/kinetics/Reaction.cpp	90.69% <98.78%> (+0.99%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19f1593...eee271c. Read the comment docs.

[speth]

The failures in the ionized flame tests aren't trivial. It looks like the logic here is causing all third body and falloff reactions containing "M" as the third body to be left out. The ch4_ion input file should create a phase with 110 reactions, but it only contains 96 with the changes in this PR.

I'm a little surprised that these are the only tests that catches this problem.

As far as the implementation here, I wonder if it would make sense to move more of this checking into the Reaction classes, rather than having different implementations of this decision tree in Kinetics::addReaction and getReactions. It could almost be part of Reaction::validate, with the wrinkle that validate doesn't currently take the Kinetics object as an argument.

[ischoegl]

The failures in the ionized flame tests aren't trivial.

Huh?! ... I remember checking on the number of reactions earlier also, but must have overlooked something here. It's pretty surprising that none of the other CI tests failed, and I had issues with IonFlame before.

Regarding the implementation: I retained the old philosophy where Reaction::validate checks for syntax, whereas Kinetics::addReaction ensures that all species are present and the reaction can actually be added. I believe it makes sense to keep this separation ...

[speth]

Given that the only place where Reaction::validate is called is in Kinetics::addReaction, I'm not sure the distinction of what kind of checks belong where is that important. I guess there is a distinction between whether a Reaction is generically valid, and whether it is fully defined for a specific Kinetics object, so maybe this doesn't belong in the validate function specifically.

But I think we could avoid having to add three new functions to the Reaction interface, and avoid the growth in complexity of both Kinetics::addReaction and getReactions, especially since the two implementations differ in ways that don't entirely make sense to me. For instance, the version in Kinetics::addReaction goes through the effort of explaining which species are undeclared, while the version in getReactions does not provide this information. Plus, that way it's only one implementation that has to be debugged to fix the current issue.

[ischoegl]

[...] I guess there is a distinction between whether a Reaction is generically valid, and whether it is fully defined for a specific Kinetics object, so maybe this doesn't belong in the validate function specifically.

This is pretty much my point, and validate should be imho called after the Reaction setup outside of Kinetics (e.g. it could be called by the factory methods newReaction). That way that separation would be much clearer - it's a little more of a change than what I had intended to do, but it would make sense.

I see your point of not wanting to add more methods to Reaction than necessary. A middle ground would be to create a new Reaction::checkSpecies method, that gets called by Kinetics::addReaction. Note that once #995 is fully implemented, there wouldn't be a need to overload, as the handling of third bodies will be more generic and everything can be handled by a single method. One advantage would be that the same logic can be used by getReactions, where I did improve output, but not to the same extent as what is done elsewhere.

[speth]

This is pretty much my point, and validate should be imho called after the Reaction setup outside of Kinetics (e.g. it could be called by the factory methods newReaction). That way that separation would be much clearer - it's a little more of a change than what I had intended to do, but it would make sense.

You could do the validation in newReaction for reactions created from YAML, but this validation also applies for reactions that are built up purely in code, in which case it's sort of hard to do it before you get to addReaction. And we wouldn't want to do the validation twice for the most common case where the reactions are generated from an input file.

I see your point of not wanting to add more methods to Reaction than necessary. A middle ground would be to create a new Reaction::checkSpecies method, that gets called by Kinetics::addReaction. Note that once #995 is fully implemented, there wouldn't be a need to overload, as the handling of third bodies will be more generic and everything can be handled by a single method. One advantage would be that the same logic can be used by getReactions, where I did improve output, but not to the same extent as what is done elsewhere.

I'm 👍 on Reaction::checkSpecies, and if it doesn't require any overloads once we get through #995, that's even better.

[ischoegl]

You could do the validation in newReaction for reactions created from YAML, but this validation also applies for reactions that are built up purely in code, in which case it's sort of hard to do it before you get to addReaction. And we wouldn't want to do the validation twice for the most common case where the reactions are generated from an input file.

Fair point. For the new structure, I started to differentiate between validation of rate parameters and reaction equations, which would make that part easier, as each can be tied to calling specific functions with known behavior. From my perspective, it would still be worthwhile to run the check immediately (i.e. prior to addReaction), no matter whether a reaction is constructed from YAML or built in code. I believe I have an idea and will suggest that once I get back to updating this PR.

[...] if it doesn't require any overloads once we get through #995, that's even better.

Unfortunately overloads will be necessary until CTI/XML support is removed as I do not anticipate porting the old loaders to the new reaction framework.

[ischoegl]

@speth ... I fixed the logic, and moved the tests as discussed (I also moved the checkReactionBalance check so it will cover getReactions). I ended up not moving the Reaction::validate calls to avoid conflicts with other ongoing work.

[speth]

For the case of ch4_ion.yaml, I'm still seeing one issue, which is that the reaction H + O2 + AR <=> HO2 + AR is still getting added, even though the phase doesn't define AR as a species. This results in a reaction where the forward_rates_of_progress entry is nan, as are the net_production_rates for H', O2andHO2`.

Interestingly, the same reaction is also added when creating a phase from ch4_ion.cti, but in that case the rate for this reaction is just zero, and the net production rates are fine. This is presumably why the ionized flame tests are working.

[ischoegl]

@speth ... thanks for catching this: my earlier fix of the logic glitch broke something that had worked before ... turns out that the logic needs to work differently for reactions with specified collision partners. It should all be good now - I also switched the unit tests to ch4_ion.yaml to make sure.

[speth]

Thanks, @ischoegl, this looks good. I have only a couple of incredibly minor comments below.