Closed ischoegl closed 10 months ago
Merging #1588 (884565b) into main (e00c23f) will decrease coverage by
0.01%
. The diff coverage is60.71%
.
@@ Coverage Diff @@
## main #1588 +/- ##
==========================================
- Coverage 70.60% 70.60% -0.01%
==========================================
Files 379 379
Lines 59144 59164 +20
Branches 21252 21263 +11
==========================================
+ Hits 41760 41770 +10
- Misses 14311 14320 +9
- Partials 3073 3074 +1
Files Changed | Coverage Δ | |
---|---|---|
include/cantera/kinetics/Kinetics.h | 32.66% <ø> (ø) |
|
interfaces/cython/cantera/reaction.pyx | 81.74% <42.85%> (-0.48%) |
:arrow_down: |
src/kinetics/Reaction.cpp | 77.83% <61.11%> (-0.67%) |
:arrow_down: |
src/kinetics/BulkKinetics.cpp | 93.65% <100.00%> (+0.01%) |
:arrow_up: |
src/kinetics/Kinetics.cpp | 74.03% <100.00%> (+0.11%) |
:arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
Thanks for the comments, @speth!
[...] I think this mostly makes sense, with one caveat.
Namely, I think that attempting to add a reaction like
2 H + CO2 <=> H2 + CO2
to a phase that doesn't containCO2
using theSolution
"from parts" constructor should not be treated as an error. The traditional behavior of theSolution
constructor has been to automatically skip unknown third bodies, and it shouldn't matter whether or not that the unknown species is the only valid third body for a reaction.
The traditional treatment of this was to not detect explicit 3rd body colliders, which means that these species show up in product/reactant maps and thus these reactions were not included.
In part, this behavior is because we don't currently provide a way for the user to specify that unknown 3rd bodies should be skipped when using this constructor. But more importantly, it's because when constructing a mechanism directly from an input file, it's much more likely that an unknown species is typo that should be detected as an error, whereas when working with
Reaction
objects, I think it's more likely that an unknown species is intentionally not part of the phase definition, and it's easier to have Cantera skip it than make the user do the additional filtering.
~I converted the errors to warnings. For example, the extract_submechanism.py
now gives a warning, but runs through without modifications:~ ... Edit: the reaction is now skipped silently
path/to/extract_submechanism.py:58: UserWarning: Skipping three-body reaction(s) with undeclared species:
- H + O2 + AR <=> HO2 + AR <three-body-Arrhenius>
Nevermind. I removed the Skipping three-body reaction(s) with undeclared species
warning as it addresses a very minor issue that nevertheless opened a can of worms in CI testing (the run-examples
runner fails if any warnings are issued).
Changes proposed in this pull request
Fix a series of bugs and edge cases that involve undeclared third-body colliders and/or implicitly defined third-body reactions.
skip-undeclared-third-bodies
to serialization (see #1403)Solution
from scratchsamples/python/kinetics/extract_submechanism.py
sample, where reaction list included invalid reactionsReaction
objects are created from reactant/productComposition
, where several invalid combinations were not caught (also, implicit third-bodies were not implemented correctly).Reaction.reactants/products
setter in Python, as they allow for undefined behaviorIf applicable, fill in the issue number this pull request is fixing
Closes #1403 Closes #1587
If applicable, provide an example illustrating new features this pull request is introducing
Checklist
scons build
&scons test
) and unit tests address code coverage