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

yaml2ck: Check if third body is in species list #1696

Merged
merged 2 commits into from
Jun 3, 2024

Conversation

corykinney
Copy link
Contributor

@corykinney corykinney commented May 16, 2024

Changes proposed in this pull request

This pull request adds a check in yaml2ck for third body efficiencies for species that might not be present in the ct.Solution object due to the skip-undeclared-third-bodies: true flag.

Closes #1683

Using the provided example from the issue referenced:

phases:
- name: test
  thermo: ideal-gas
  elements: [O, H, C, Ar]
  species:
  - gri30.yaml/species: all
  kinetics: gas
  skip-undeclared-elements: true
  skip-undeclared-third-bodies: true
  reactions:
  - gri30.yaml/reactions: declared-species
  transport: mixture-averaged
  state: {T: 300.0, P: 1 atm}

The outputted Chemkin file from python -m cantera.yaml2ck test.yaml --overwrite before the change:

H + O2 + M <=> HO2 + M           2.8000000000000005e+18 -0.86 0.0
AR/0.000E+00/ C2H6/1.500E+00/ CO/7.500E-01/ CO2/1.500E+00/ H2O/0.000E+00/ N2/0.000E+00/ O2/0.000E+00/

versus after the change:

H + O2 + M <=> HO2 + M           2.8000000000000005e+18 -0.86 0.0
AR/0.000E+00/ C2H6/1.500E+00/ CO/7.500E-01/ CO2/1.500E+00/ H2O/0.000E+00/ O2/0.000E+00/

The N2 efficiency is omitted from the list as desired.

Feedback

An example file still needs to be added with a corresponding test case, but before doing so, I wanted to solicit feedback on potential edge cases. What behavior would be expected if no explicit third body species are present in the mechanism? Are there any other possible cases that need to be accounted for proactively?

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

Does not write the third body efficiencies for species that are not in the solution's species list
Fixed Cantera#1683
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, @corykinney. I think this looks like a clean fix. Could you please add a test to test_convert.py verifying that this works as intended?

@speth
Copy link
Member

speth commented Jun 1, 2024

What behavior would be expected if no explicit third body species are present in the mechanism?

I didn't try it, but I think your patch would add a blank line to the output. I don't see a problem with that.

- Converted species names to set comprehension, as suggested
- Added phase definition from original issue for test
- Implemented test to check that only the undeclared third body was omitted
- Temperature and pressure conditions used for kinetics check matched to GRI 3.0 test function
@corykinney
Copy link
Contributor Author

@speth I implemented your suggestions and added the necessary test case. It should be good to go, but let me know if I missed anything in the test.

Copy link

codecov bot commented Jun 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.69%. Comparing base (d37a76b) to head (39816e4).
Report is 19 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1696      +/-   ##
==========================================
- Coverage   75.69%   75.69%   -0.01%     
==========================================
  Files         443      443              
  Lines       60975    60971       -4     
  Branches     9551     9552       +1     
==========================================
- Hits        46158    46154       -4     
  Misses      11786    11786              
  Partials     3031     3031              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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, @corykinney! This looks good to me.

@speth speth merged commit 1ed2b28 into Cantera:main Jun 3, 2024
47 checks passed
@corykinney corykinney deleted the yaml2ck-undeclared-third-body branch June 11, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

yaml2ck does not remove third body efficiencies for undeclared species
2 participants