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
Remove reaction specializations #1333
Remove reaction specializations #1333
Conversation
f9208d1
to
645a31a
Compare
7b6258d
to
82bb4fd
Compare
8c2dab8
to
116dcdb
Compare
b9c7715
to
b7cb764
Compare
Rebased and updated to incorporate changes to |
b7cb764
to
e42b6b2
Compare
e42b6b2
to
b3740a8
Compare
There was a problem hiding this 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 is an exciting step forward in simplifying the handling of different reaction rate parameterizations.
One thing that you only barely touched on is that by handling third bodies in the common Reaction
class, these can in principle be used for any rate parameterization. However, there are a couple of quirks here that I think are worth ironing out. One is that there are a few places that single out the TwoTempPlasmaRate
class as not being able to handle third bodies, but I don't understand why that would be the case now. Shouldn't any reaction type be able to handle third bodies now, with the exception that we explicitly remove them from Plog and Chebyshev reactions in acknowledgment that the third-body pressure dependence is already embedded in those rate constants?
The second is that there are a few situations where the automatic detection of specific third body colliders prevents instantiation of reactions as non-third-body reactions, even though I don't think these should be seen as problematic. For instance, the following reaction definitions raise exceptions, but were (I think correctly) permitted in Cantera 2.6:
R1 = ct.Reaction(
equation="HO2 + O2 <=> OH + O + O2",
rate={"type": "pressure-dependent-Arrhenius",
"rate-constants": [
{"P": 1013.25, "A": 1.2124e+16, "b": -0.5779, "Ea": 45491376.8},
{"P": 101325., "A": 4.9108e+31, "b": -4.8507, "Ea": 103649395.2}
],
}
)
R2 = ct.Reaction(
equation="H + OH + 2 O2 = H2O + 2 O2",
rate={"A": 1.234e5, "b": 0, "Ea": 4321.0}
)
36a27db
to
fbfbc13
Compare
@speth ... thanks for the feedback. I ended up being a lot more permissive/selective with the detection of third bodies, which I believe does make sense. Regarding the two reactions you mentioned, they now produce the following:
|
fbfbc13
to
1b5210f
Compare
c49e7fd
to
55ee7e8
Compare
@speth ... I agree that there should be some updates to the Science section, but I think the rest is done! Thank you for the suggestions ... this should complete the work left over on refactoring of |
034f4ca
to
2efa9b8
Compare
2efa9b8
to
b3d0d9a
Compare
@speth ... I opened Cantera/cantera-website#216 to track the remaining documentation work. |
There was a problem hiding this 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 is good to go.
Changes proposed in this pull request
ThreeBodyReaction
in C++ and PythonFalloffReaction
in C++ and PythonReactionFactory
and deprecatenewReaction
constructorsReaction.__repr__
outputThirdBody
object in Python and deprecateefficiencies
argument in constructorIncorporates #1332If applicable, fill in the issue number this pull request is fixing
Closes #1220; partially addresses Cantera/enhancements#142
If applicable, provide an example illustrating new features this pull request is introducing
Comments
three-body
type entry is now optional in YAML inputBlowers-Masel
ratesChecklist
scons build
&scons test
) and unit tests address code coverage