-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
Inconsistency in ReactionRateCoeff units calculation #1071
Comments
Thanks for reporting this @ischoegl Can you provide a more concrete example of how this goes wrong, or what the problem is? I didn't get it from your initial description there. Thanks! |
This isn't entirely true. The following bit of // For reactions created outside the context of a Kinetics object, the units
// of the rate coefficient can't be determined in advance. Do that here.
if (r->rate_units.factor() == 0) {
r->calculateRateCoeffUnits(*this);
} So the only case where the rate coefficient units aren't determined is when the reactant and product species names have never been bound to species in real phases. |
@speth ... thanks for pointing this out! This looks familiar, but I had not remembered when creating this issue report. One question here that has been nagging me forever is why not defer the calculation of the rate units factor for all reactions until they're added? That way, you wouldn't need to pass a kinetics argument (at least for the purpose of calculating units). This would also avoid the (currently possible) scenario where reactions are created based on kinetics object A but attached to kinetics object B, with incompatible reacting phase standard concentrations. As an aside, I am aware of other scenarios where kinetics information is required a priori (e.g. electrochemical rxn's), which is a separate issue and can be addressed at a later point. |
In the case where the reaction and rate object are created directly in code, the rate coefficient is by definition in Cantera's MKS+kmol unit system. All that's unknown is the dimensions of the rate coefficient. However, when you create a reaction from a YAML definition, that's not the case. The
I think that using the same set of rate parameters in this case is physically inconsistent. But short of requiring that the parameters in the rate constant expression be given with their often complex units, I don't see how we can prevent the incorrect use of rate constants in such a scenario. |
@speth ... I opened #1076 to have some means to discuss this further. I'm not exactly sure that I completely understand everything here, but it doesn't appear that setting the units before or afterwards has much of an impact on the numeric value. (I do understand that the unit system does play a role when importing YAML though):
Creating a duplicate for
And adding it to
So there isn't a way to set an alternative unit system when creating the reaction, and attaching it to the PS: I figure that |
@speth ... Reflecting on this some more, I believe the most consistent way to handle units would be to pass a |
Yes, that is correct, all of the unit conversions are handled at the time the I think providing just a - equation: O2 + 2 PT(S) => 2 O(S) # Reaction 4
rate-constant: {A: 1.8e+21, b: -0.5, Ea: 0}
duplicate: true you need to know that:
None of this can be determined from a |
@speth ... thanks for confirming! The example you provided cleared up some things for me. I agree that it won't be possible to set rxn = ct.InterfaceReaction(equation="O2 + 2 PT(S) => 2 O(S)",
rate={"A": 1.8e+21, "b": -0.5, "Ea": 0.0}, kinetics=surf) which is equivalent to the existing rxn = ct.InterfaceReaction(reactants={"O2": 1, "PT(S)": 2}, products={"O(S)": 2})
rxn.rate = ct.Arrhenius(1.8e+17, -0.5, 0.0) where for the latter, m_A = units.convert(rate_map["A"], rate_units); which currently only works for creation from Incidentally, there appears to be a bug even for the YAML route
as the new reaction should be a duplicate of reaction 4. As another aside, this nonsensical bit currently also works: rxn2 = ct.Reaction.fromYaml("""
equation: O2 + 2 PT(S) => 2 O(S) # Reaction 4
rate-constant: {A: 1.8e+21, b: -0.5, Ea: 0}
""", kinetics=ct.Solution('gri30.yaml')) where the unit calculation fails silently. |
Sorry if I have another question in this context. Looking at units in |
Why is the numerical value for A different in these two examples? As written, I think both of those values must be written in the MKS+kmol unit system, since there's nothing to indicate otherwise. If you're willing to defer the calculation of the rate constant units, then I guess the first case should be possible without providing the rxn = ct.InterfaceReaction(equation="O2 + 2 PT(S) => 2 O(S)",
rate={"A": "1.8e+31 cm^5/kmol^2/s", "b": -0.5, "Ea": 0.0}, kinetics=surf)
I agree, we do seem to have gotten ourselves into a bit of a messy situation here. I think what's confusing is that right now, both the
I think the best way to provide some flexibility of working with alternative units while keeping Cantera grounded in a consistent unit system is to support alternative units, for reaction rates or anything else, only when using YAML/
Careful - if you copy the reaction specification without bringing along the corresponding rxn2 = ct.Reaction.fromYaml("""
equation: O2 + 2 PT(S) => 2 O(S) # Reaction 4
units: {length: cm, quantity: mol, activation-energy: J/mol}
rate-constant: {A: 1.8e+21, b: -0.5, Ea: 0}""",
kinetics=surf)
Right, the [standard] concentration in the gas phase has dimensions of quantity/length^3, which means kmol/m^3 in Cantera, and the units in >>> gri.reaction(2).rate.pre_exponential_factor
38.7 There's nothing implicit about this conversion. |
You are correct - they should be the same (and different from YAML). Neither variant is aware of a
While I am anticipating to move
... makes sense! So it looks like YAML already allows for this (failed to connect the dots for the
Ha! My mistake - didn't realize that all of the values in the docstring examples already had the conversion applied. I think I'm finally on the correct page here, but am still not satisfied with the 'from-scratch' API. |
At least for the Python interface, the second option here would be very straightforward. All that's needed is to accept a |
Absolutely!
Yes. But it could be optional, with conversion taking place when a |
I think having the However, I do not think that we should defer converting values to Cantera's unit system. That would be a substantial departure from how dimensional values are handled throughout the rest of Cantera, where those values are almost always expressed in the MKS+kmol system, and certainly don't change dynamically. If I'm understanding your suggestion here correctly, the following would yield two different values for the rate constant: >>> rxn1 = ct.InterfaceReaction(
equation="O2 + 2 PT(S) => 2 O(S)",
units={"length": "cm", "quantity": "mol", "activation-energy": "J/mol"},
rate={"A": 1.8e+21, "b": -0.5, "Ea": 0})
>>> surf.add_reaction(rxn1)
>>> rxn2 = surf.reactions()[-1]
>>> print(rxn1.rate.pre_exponential_factor)
>>> print(rxn2.rate.pre_exponential_factor)
1.8e+21
1.8e+17 If that's the case, I see a lot of potential for errors that we can avoid by just requiring a |
I agree to the extent possible (I don't see a way for
Once attached, they should yield the same result. So (using #1076 for >>> rxn1 = ct.InterfaceReaction(
equation="O2 + 2 PT(S) => 2 O(S)",
units={"length": "cm", "quantity": "mol", "activation-energy": "J/mol"},
rate={"A": 1.8e+21, "b": -0.5, "Ea": 0})
>>> print(rxn1.rate.pre_exponential_factor)
1.8e+21
>>> rxn1.rate_coeff_units
<Units(0.0) at 7ff3cd17a5d0> and >>> surf.add_reaction(rxn1)
>>> rxn2 = surf.reactions()[-1]
>>> print(rxn1.rate.pre_exponential_factor)
1.8e+17
>>> print(rxn2.rate.pre_exponential_factor)
1.8e+17
>>> rxn1.rate_coeff_units
<Units(1.0 kmol^-2 * m^5 * s^-1) at 7ff3cd16adb0> Regarding the comment for the pre-existing pathway, I also suggest the following >>> rxn1 = ct.InterfaceReaction(reactants={"O2": 1, "PT(S)": 2}, products={"O(S)": 2})
>>> rxn1.rate = ct.ArrheniusRate(
A=1.8e+21, b=-0.5, Ea=0,
units={"length": "cm", "quantity": "mol", "activation-energy": "J/mol"})
>>> print(rxn1.rate.pre_exponential_factor)
1.8e+21
>>> rxn1.rate_coeff_units
<Units(0.0) at 7ff3cd17a5d0> with the same behavior as above. I believe that this would ensure that users don't get stuck with always having to convert non MKS+kmol units manually (which is one of my concerns). PS: I think it should be possible to leave PPS: I also believe that this should raise an error rxn2 = ct.Reaction.fromYaml("""
equation: O2 + 2 PT(S) => 2 O(S) # Reaction 4
rate-constant: {A: 1.8e+21, b: -0.5, Ea: 0}
""", kinetics=ct.Solution('gri30.yaml')) |
There is a perfectly robust way to avoid deferring the conversion of A: instantiate the
Sorry, you're right - my example was flawed and of course the two Python objects should be sharing the same underlying C++ But I don't think we should introduce the possibility that the value returned by |
This is true for
The main benefit here would be that it allows for |
Agreed. And for this purpose, we already have the constructors for
Perhaps you have a different vision for how this could be implemented, but my initial take is that even for a simple rate type like
In the absence of a definition for a reaction (including information about the phase of each reactant) or a specification of units (not just unit system), I don't think a rate constant has any meaning beyond being an arbitrary mathematical expression. If you want to use it like that, the |
@speth ... thanks for your comments.
👍 Ok. I believe this addresses the sticking point in #1061, where my latest update for
This is the most logical implementation. If we agree on that, various constructors could accommodate a
I agree that rate constants are by definition associated with a reaction. My concern here is mainly to Regardless of the final verdict here, I believe there's a clear path forward at this point:
|
Problem description
Since the introduction of YAML, reaction constructors ensure that units are provided in a consistent manner. Units are, however, not consistently handled when constructing reactions from scratch, i.e.
or
as well as all other corresponding constructor instances.
While it is (at least for the newer framework) possible to associate units with
ReactionRate
objects, there never is a call toReaction::calculateRateCoeffUnits
that would otherwise handle units.Per @speth's comments ...
Steps to reproduce
N/A ... although these C++ constructors are widely used in the C++ test suite.
Behavior
Units are never calculated (no call to
Reaction::calculateRateCoeffUnits
).System information
Additional context
This issue surfaced when discussing PR #1061
The text was updated successfully, but these errors were encountered: