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

Incorporating the LMR-R mixture rule into Cantera for the first time #1710

Open
wants to merge 145 commits into
base: main
Choose a base branch
from

Conversation

pjsingal
Copy link

@pjsingal pjsingal commented Jun 12, 2024

Changes proposed in this pull request

  • LmrRate.cpp and LmrRate.h allow for vastly improved consideration of the mixture-dependent aspect of the rate constants for complex-forming ("pressure-dependent") reactions, using the new LMR-R mixture theory developed by the Burke Lab at Columbia University

  • This code builds, runs, and produces results that have been validated and peer-reviewed

  • LMR-R can handle three different representations of the pressure-dependent aspect of complex-forming reactions: Troe, PLOG, and Chebyshev

  • LMR-R calculates the mixture-dependent aspect using temperature-dependent third-body efficiencies that have been supplied by the user for at least one collider - these parameters can be provided in either relative or absolute terms, as long as the user's choice is consistent for all colliders in a given reaction

  • Description of the theory and instructions for implementation have been added to rate-constants.md and reactions.rst, respectively

  • Four new Python examples have been added

Checklist

[x] The pull request includes a clear description of this code change
[x] Commit messages have short titles and reference relevant issues
[x] Build passes (scons build & scons test) and unit tests address code coverage
[x] Style & formatting of contributed code follows contributing guidelines
[x] The pull request is ready for review

@speth speth added the Kinetics label Jun 12, 2024
@pjsingal pjsingal changed the title Burkelab clean Incorporating the LMR-R mixture rule into Cantera for the first time Jun 12, 2024
Copy link

codecov bot commented Jun 12, 2024

Codecov Report

Attention: Patch coverage is 0.78740% with 252 lines in your changes missing coverage. Please review.

Project coverage is 72.87%. Comparing base (ec0cfdb) to head (0eb6e58).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/kinetics/LinearBurkeRate.cpp 0.00% 224 Missing ⚠️
include/cantera/kinetics/LinearBurkeRate.h 0.00% 15 Missing ⚠️
interfaces/cython/cantera/reaction.pyx 14.28% 12 Missing ⚠️
src/kinetics/ReactionRateFactory.cpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1710      +/-   ##
==========================================
- Coverage   73.23%   72.87%   -0.37%     
==========================================
  Files         381      383       +2     
  Lines       54375    54616     +241     
  Branches     9251     9282      +31     
==========================================
- Hits        39823    39802      -21     
- Misses      11580    11832     +252     
- Partials     2972     2982      +10     

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

Copy link
Member

@ischoegl ischoegl 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 this contribution, @pjsingal! I have a couple of basic recommendations that mostly pertain to formatting. The example data should likely move to the new example_data repository, but it is not urgent at the moment. You may also want to think about adding unit tests.

.gitignore Outdated Show resolved Hide resolved
data/alzueta_LMRR.yaml Outdated Show resolved Hide resolved
data/alzueta_LMRR.yaml Outdated Show resolved Hide resolved
data/example_data Outdated Show resolved Hide resolved
ext/googletest Outdated Show resolved Hide resolved
samples/python/kinetics/flame_speed.py Outdated Show resolved Hide resolved
samples/python/kinetics/flame_speed.py Outdated Show resolved Hide resolved
samples/python/kinetics/jet_stirred_reactor.py Outdated Show resolved Hide resolved
@pjsingal pjsingal force-pushed the burkelabClean branch 2 times, most recently from ee0a85f to 780c996 Compare September 5, 2024 17:27
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 for the continued work on this, @pjsingal. I would suggest that the first priority at this point is to add some unit tests. I would recommend getting those into place before making any of the other changes that I've suggested below, so you can be sure that making those changes doesn't introduce any regressions.

data/alzueta.yaml Outdated Show resolved Hide resolved
samples/python/kinetics/ignition_delay_time.py Outdated Show resolved Hide resolved
samples/python/kinetics/flame_speed.py Outdated Show resolved Hide resolved
include/cantera/kinetics/LinearBurkeRate.h Outdated Show resolved Hide resolved
include/cantera/kinetics/LinearBurkeRate.h Outdated Show resolved Hide resolved
Comment on lines +162 to +169
// Collider has an eps specified, but no other info is provided. Assign it the same rate and data objects as "M"
else {
rateObjs.push_back(rateObj_M);
dataObjs.push_back(dataObj_M);
epsObjs1.push_back(epsObj_i);
epsObjs2.push_back(epsObj_M);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Since this seems to be the most common case (based on the mechanism used in the examples), for the sake of efficiency I think you want to avoid recalculating the value of rateObj_M for each collider when (I think) it will just have the same value for all of them and all you really need is to compute it once and scale it by the sum of the efficiency-weighted mole fractions.

Copy link
Author

Choose a reason for hiding this comment

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

This case actually only applies to colliders which have been explicitly specified with an eig0 or eps value but lack a k(TP) parameterization. For example, in the reaction H + O2 (+M) <=> HO2 (+M) this applies to only He, N2, H2, CO2, NH3, and H2O (thereby leaving 36 other species to be calculated later on in one step using the mole fraction sum). If Troe/PLOG/Chebyshev were provided for He, e.g., then it would be treated using the other branches of this conditional structure. Modifying this aspect of the code would make evalFromStruct() more complicated later on. I would prefer to leave it as-is, but am happy to modify it if you think the computational cost is too high.

doc/example-keywords.txt Outdated Show resolved Hide resolved
doc/example-keywords.txt Outdated Show resolved Hide resolved
Comment on lines 680 to 684
elif input_data:
raise TypeError("Invalid parameter 'input_data'")
else:
raise TypeError("Invalid parameter 'rates'")
self.set_cxx_object()
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to get the constructor from input_data working by following the example of the other rate types. This is just a Python data structure containing the same information as the YAML input.

I think you can just remove the rates kwarg -- I'm guessing that's just leftover from copying from the PlogRate implementation.

include/cantera/kinetics/LinearBurkeRate.h Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants