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

Cantera version 2.6 #2288

Merged
merged 5 commits into from
May 8, 2023
Merged

Cantera version 2.6 #2288

merged 5 commits into from
May 8, 2023

Conversation

ChrisBNEU
Copy link
Contributor

@ChrisBNEU ChrisBNEU commented Mar 28, 2022

Motivation or Problem

addresses issue #2270.
Cantera is deprecating the ".cti" output file type that RMG currently uses. for nicer features like coverage dependant thermo and kinetics, we need to upgrade cantera to v2.5 and switch to the yaml output file type.

Description of Changes

Changed the input parser from ck2cti to ck2yaml. there were some syntax changes for the parser from v2.3 to 2.5.

Testing

update all of the unit tests that involve writing or reading a cantera output file
ensure that a ct.Solution and ct.interface object load correctly externally.

Reviewer Tips

Any suggestions for what should be tested would be appreciated, I have not put any in.

@ChrisBNEU ChrisBNEU changed the title missed several syntax changes, updated Cantera version 2.5 Mar 28, 2022
@ChrisBNEU
Copy link
Contributor Author

currently generated a chem_annotated.yaml and a chem.yaml. chem.yaml looks fine but the chem_annotated.yaml does not store the comments under "notes" for the reactions.

@codecov
Copy link

codecov bot commented Aug 17, 2022

Codecov Report

Merging #2288 (c149f79) into main (f8b136e) will decrease coverage by 0.04%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##             main    #2288      +/-   ##
==========================================
- Coverage   48.15%   48.12%   -0.04%     
==========================================
  Files         110      110              
  Lines       30654    30653       -1     
  Branches     7994     7995       +1     
==========================================
- Hits        14762    14751      -11     
- Misses      14357    14372      +15     
+ Partials     1535     1530       -5     
Impacted Files Coverage Δ
rmgpy/tools/canteramodel.py 33.59% <81.48%> (-2.80%) ⬇️
rmgpy/rmg/main.py 22.09% <85.71%> (ø)

... and 1 file with indirect coverage changes

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

@ChrisBNEU ChrisBNEU marked this pull request as ready for review August 17, 2022 19:38
@ChrisBNEU
Copy link
Contributor Author

ChrisBNEU commented Aug 18, 2022

This passes all tests and fixes the issue of not getting the coverage dependent parameters into the Cantera output file. I believe this needs to be merged before we can merge #2321.

@ChrisBNEU
Copy link
Contributor Author

One other thing to note is the test for generating an output file with the ck2yaml parser behaves a little differently. If we do not specify all of the elements used (e.g. we forget to add "C" for our carbon atom in the elements list) it will not throw an error. The error only appears when the output file is validated, which is not something cantera coded into the parser (you can do it with the command line ck2yaml parser however). I just changed the test case so it fails for another reason, since the point is not to exhaustively test every error possible when generating a yaml file. see this thread for more info: https://groups.google.com/g/cantera-users/c/eiTX-2O5zfQ

@ChrisBNEU
Copy link
Contributor Author

upgrading to version 2.6 in environment.yml

@calvinp0
Copy link
Member

calvinp0 commented Mar 2, 2023

Hey @ChrisBNEU, with this branch, are you experiencing an issue with installing PyCall in Julia (or add/installing new packages in Julia)

@calvinp0
Copy link
Member

calvinp0 commented Mar 2, 2023

A fix for that issue is we need to make it use the libcurl packaged with Julia.

For my env, I had to do this

export LD_LIBRARY_PATH="/home/calvin/mambaforge/envs/rmg_env/share/julia/site/julia-1.6.7/lib/julia:$LD_LIBRARY_PATH"

@calvinp0
Copy link
Member

calvinp0 commented Mar 2, 2023

A fix for that issue is we need to make it use the libcurl packaged with Julia.

For my env, I had to do this

export LD_LIBRARY_PATH="/home/calvin/mambaforge/envs/rmg_env/share/julia/site/julia-1.6.7/lib/julia:$LD_LIBRARY_PATH"

@mjohnson541 - Return of the LD_LIBRARY_PATH issue

@JacksonBurns
Copy link
Contributor

I am rebasing this branch to try and push this feature through now that the CI has been fixed.

@ChrisBNEU ChrisBNEU changed the title Cantera version 2.5 Cantera version 2.6 May 3, 2023
@ChrisBNEU ChrisBNEU force-pushed the cantera_v2_5 branch 2 times, most recently from 58fba74 to dc73217 Compare May 5, 2023 18:27
@ChrisBNEU
Copy link
Contributor Author

So there are a number of new issues with cantera 2.6 and beyond that have done my best to address:

Older versions of cantera used different Reaction objects for each reaction type (like ArrheniusReaction, PlogReaction, InterfaceReaction, etc.). There was a concerted effort to consolidate all reactions into the cantera.Reaction object, and differentiate with new ReactionRate objects (e.g. LindemannRate, PlogRate, InterfaceArrheniusRate). This was partially implemented in 2.6, but it is not fully implemented for ThirdBodyReaction types. These use the ThirdBodyReaction type, but with a ThirdBodyRate.

FalloffReaction is used for Lindemann and Troe reactions because it is what cantera.Reaction.from_yaml and cantera.Solution(<yaml_file>) creates in v2.6. In v3.0, cantera.Reaction.from_yaml will correctly create cantera.Reaction objects with TroeRate and LindemannRate. The reactionTest.py unit test is written such that it will only look at the rate, which will remain the same in v3.0

For versions <=3.0, we will not need to worry about third body reactions and falloff reactions. however, past version 3.0, we will need to recode so all reactions are type cantera.Reaction. Third body reactions will need a third_body object specified for the Reaction() object. For more info on reactions and rates in the newer release of cantera, refer to the documentation.

All cti output file references are removed, since cti references are deprecated in v2.6, and will not work in later versions. additionally, cti files will not correctly write coverage dependencies, but yaml files will.

I have not updated the surface reaction to_cantera methods. they did not exist before, and they require a fair bit of additional coding for unit conversion. I think that should live in a separate pr.

@ChrisBNEU ChrisBNEU added the Status: Ready for Review PR is complete and ready to be reviewed label May 5, 2023
Copy link
Contributor

@JacksonBurns JacksonBurns left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, implementation looks great! I have a couple minor comments on documentation and clarity.

One larger thing is that this does break compatibility with previous versions of RMG since we can no longer interface with the 'cti' format from Cantera. We should bundle this into a minor release of RMG-Py, like 3.3.0 or the like.

rmgpy/kinetics/arrhenius.pyx Outdated Show resolved Hide resolved
rmgpy/kinetics/chebyshev.pyx Outdated Show resolved Hide resolved
rmgpy/kinetics/falloff.pyx Outdated Show resolved Hide resolved
rmgpy/reaction.py Outdated Show resolved Hide resolved
Copy link
Member

@rwest rwest left a comment

Choose a reason for hiding this comment

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

Thanks Chris. Lots of work here.
I agree with @JacksonBurns that forcing change to 2.6 and yaml from 2.3 and cti is likely a major change for some users, but I can't see a way for both options to co-exist. (or at least not a way that is simple enough to be worth pursuing).

Note regarding my review: I just read the code on github and haven't actually tried running any of it.

rmgpy/kinetics/falloff.pyx Outdated Show resolved Hide resolved
rmgpy/kinetics/arrhenius.pyx Outdated Show resolved Hide resolved
rmgpy/tools/data/various_kinetics/chem_annotated.inp Outdated Show resolved Hide resolved
Copy link
Contributor

@JacksonBurns JacksonBurns left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for making this important PR happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Ready for Review PR is complete and ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants