Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues for undeclared third-bodies and implicit third-body reactions #1588

Merged
merged 7 commits into from
Aug 15, 2023

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Aug 12, 2023

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.

  • Add skip-undeclared-third-bodies to serialization (see Unable to load serialized mechanism where reactions contain undeclared third bodies #1403)
  • Prevent segfault if an implicitly defined third-body reaction contains an invalid collider (see Segfault with undefined/explicit third-body collider #1587)
  • Prevent silent skipping of invalid reactions in Python API for creation of Solution from scratch
  • Fix samples/python/kinetics/extract_submechanism.py sample, where reaction list included invalid reactions
  • Address several edge cases where Reaction objects are created from reactant/product Composition, where several invalid combinations were not caught (also, implicit third-bodies were not implemented correctly).
  • Deprecate Reaction.reactants/products setter in Python, as they allow for undefined behavior

If 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

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference 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

@ischoegl ischoegl added the Input Input parsing and conversion (for example, ck2yaml) label Aug 12, 2023
@codecov
Copy link

codecov bot commented Aug 12, 2023

Codecov Report

Merging #1588 (884565b) into main (e00c23f) will decrease coverage by 0.01%.
The diff coverage is 60.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%) ⬇️
src/kinetics/Reaction.cpp 77.83% <61.11%> (-0.67%) ⬇️
src/kinetics/BulkKinetics.cpp 93.65% <100.00%> (+0.01%) ⬆️
src/kinetics/Kinetics.cpp 74.03% <100.00%> (+0.11%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ischoegl ischoegl marked this pull request as ready for review August 12, 2023 17:55
@ischoegl ischoegl changed the title Undeclared thirdbodies Fix issues for undeclared third-bodies and implicit third-body reactions Aug 12, 2023
@ischoegl ischoegl requested a review from a team August 12, 2023 19:04
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks, @ischoegl. 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 contain CO2 using the Solution "from parts" constructor should not be treated as an error. The traditional behavior of the Solution 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.

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.

interfaces/cython/cantera/reaction.pyx Outdated Show resolved Hide resolved
interfaces/cython/cantera/reaction.pyx Show resolved Hide resolved
interfaces/cython/cantera/solutionbase.pyx Outdated Show resolved Hide resolved
src/kinetics/Reaction.cpp Outdated Show resolved Hide resolved
src/kinetics/Reaction.cpp Outdated Show resolved Hide resolved
test/python/test_kinetics.py Outdated Show resolved Hide resolved
samples/python/kinetics/extract_submechanism.py Outdated Show resolved Hide resolved
@ischoegl
Copy link
Member Author

ischoegl commented Aug 15, 2023

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 contain CO2 using the Solution "from parts" constructor should not be treated as an error. The traditional behavior of the Solution 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>

@ischoegl
Copy link
Member Author

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

Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks, @ischoegl. These updates look good to me. Feel free to merge once the CI passes.

@ischoegl ischoegl merged commit 717af98 into Cantera:main Aug 15, 2023
40 of 42 checks passed
@ischoegl ischoegl deleted the undeclared-thirdbodies branch August 15, 2023 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Input Input parsing and conversion (for example, ck2yaml) Kinetics
Projects
No open projects
2 participants