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

Inconsistency in ReactionRateCoeff units calculation #1071

Closed
ischoegl opened this issue Jul 15, 2021 · 20 comments · Fixed by #1061
Closed

Inconsistency in ReactionRateCoeff units calculation #1071

ischoegl opened this issue Jul 15, 2021 · 20 comments · Fixed by #1061

Comments

@ischoegl
Copy link
Member

ischoegl commented Jul 15, 2021

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.

ElementaryReaction3(const Composition& reactants, const Composition& products, const ArrheniusRate& rate);

or

ElementaryReaction2(const Composition& reactants_, const Composition products_, const Arrhenius& rate_);

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 to Reaction::calculateRateCoeffUnits that would otherwise handle units.

Per @speth's comments ...

the units for the rate constant are a combination of the reacting phase standard concentration units and the concentration units for the reactants, as in Reaction::calculateRateCoeffUnits.

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

  • Cantera version: current development version
  • OS: all
  • Python/MATLAB/other software versions: N/A (it is part of the C++ core)

Additional context

This issue surfaced when discussing PR #1061

@ischoegl ischoegl mentioned this issue Jul 15, 2021
8 tasks
@ischoegl ischoegl changed the title Inconsistency in Reaction units calculation Inconsistency in ReactionRateCoeff units calculation Jul 15, 2021
@bryanwweber
Copy link
Member

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!

@speth
Copy link
Member

speth commented Jul 16, 2021

While it is (at least for the newer framework) possible to associate units with ReactionRate objects, there never is a call to Reaction::calculateRateCoeffUnits that would otherwise handle units.

This isn't entirely true. The following bit of Kinetics::addReaction does just that:

    // 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.

@ischoegl
Copy link
Member Author

@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.

@speth
Copy link
Member

speth commented Jul 16, 2021

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).

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 input property of the Reaction does carry that UnitSystem definition, so you could defer the conversion, but I think it would be pretty strange for the numerical value returned by Reaction.rate(T) to change when that reaction is added to a Kinetics object.

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.

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.

@ischoegl
Copy link
Member Author

ischoegl commented Jul 17, 2021

@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):

In [1]: import cantera as ct
   ...: gas = ct.Solution("gri30.yaml") 
   ...: # units: {length: cm, time: s, quantity: mol, activation-energy: cal/mol}
   ...: gas.reaction(2).input_data
   ...: 
Out[1]: 
{'equation': 'H2 + O <=> H + OH',
 'rate-constant': {'A': 38.7, 'Ea': 26191840.0, 'b': 2.7}}

In [2]: gas.reaction(2).rate_coeff_units
Out[2]: 'Units(1.0 kmol^-1 * m^3 * s^-1)'

In [3]: gas.reaction(2).rate(300)
Out[3]: 5195.44737317942

Creating a duplicate for gas.reaction(2) ...

In [4]: reactants = {'O': 1., 'H2': 1}
   ...: products = {'H': 1., 'OH': 1.}
   ...: rxn = ct.ElementaryReaction(reactants, products)
   ...: rxn.rate = ct.ArrheniusRate(A=38.7, b=2.7, Ea=26191840.0) # use standard units
   ...: rxn
   ...: 
Out[4]: <ElementaryReaction: H2 + O <=> H + OH>

In [5]: rxn.rate_coeff_units
Out[5]: 'Units(0.0)'

In [6]: rxn.rate(300) # evaluate without having any unit information attached
Out[6]: 5195.44737317942

And adding it to gas

In [7]: gas.add_reaction(rxn)
   ...: rxn.rate_coeff_units
   ...: 
Out[7]: 'Units(1.0 kmol^-1 * m^3 * s^-1)'

In [8]: rxn.rate(300) # evaluate with unit information attached
Out[8]: 5195.44737317942

In [9]: rxn2 = gas.reaction(gas.n_reactions - 1) # just to make sure we're not dealing with a copy
   ...: rxn2.rate(300.)
   ...:
Out[9]: 5195.44737317942

In [10]: gas.forward_rate_constants[[2,-1]] # no difference here either
Out[10]: array([5195.44737318, 5195.44737318])

In [11]: gas.standard_concentration_units
Out[11]: 'Units(1.0 kmol * m^-3)'

So there isn't a way to set an alternative unit system when creating the reaction, and attaching it to the Kinetics object does not affect numerical values? (From what I understand, all unit conversions are completed when a Reaction is created?)

PS: I figure that standardConcentrationUnits are set to the default here (despite the fact that units for quantity are specified as mol for the input file). Can you point me to examples where this is not the case? I.e. is there any case where it is not a combination of kmol and m with factor 1.0?

@ischoegl
Copy link
Member Author

@speth ... Reflecting on this some more, I believe the most consistent way to handle units would be to pass a UnitSystem instead of the Kinetics object when creating a Reaction (or ReactionRate) from scratch. I exposed UnitSystem to Python in #1076, which would be the basis for what - I believe - would be a more consistent approach.

@speth
Copy link
Member

speth commented Jul 19, 2021

Yes, that is correct, all of the unit conversions are handled at the time the Reaction is created, which means that the numerical values returned by Reaction.rate will always be in the MKS+kmol system, and that is why the factor for the rate_units is always 1.0. I don't think we have any cases where the rate constant units are weirder than kmol^a * m^b * s^-1 where a and b are real numbers, although that's not a strict requirement.

I think providing just a UnitSystem is insufficient for creating a Reaction from a YAML definition (if unit conversions are required) or if you want to correctly set the rate_units variable when creating a reaction "from scratch". For instance, to determine the units of the following reaction from ptcombust.yaml

- 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:

  • the concentration units of the reacting phase have dimensions of quantity/length^2
  • the concentration units for O2 are quantity/length^3
  • the concentration units for PT(S) are quantity/length^2

None of this can be determined from a UnitSystem alone.

@ischoegl
Copy link
Member Author

ischoegl commented Jul 20, 2021

@speth ... thanks for confirming! The example you provided cleared up some things for me. I agree that it won't be possible to set rate_units without Kinetics, but things nevertheless do not look quite consistent to me. But to continue: while the following isn't implemented yet, this is anticipated after Cantera/enhancements#87

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, rate_units are set when the reaction is attached to a Kinetics object, and the A factor has to be adjusted manually as there is no automatic conversion. I guess it boils down to this line for Arrhenius::setParameters in RxnRates.cpp

m_A = units.convert(rate_map["A"], rate_units);

which currently only works for creation from AnyMap. What somewhat bothers me here is that there is a circular dependency between Reaction and ReactionRate, which is not ideal. Imho, it would be great if we could come up with a way to handle this in a consistent fashion, as the manual unit adjustments (see above) aren't necessarily intuitive. (I believe it's actually more problematic than Reaction.rate(T) changing after attaching a Reaction to Kinetics). Beyond, it would be neat to allow for alternative unit systems when defining ReactionRate objects from scratch).

Incidentally, there appears to be a bug even for the YAML route

In [2]: gas = ct.Solution('ptcombust.yaml')
   ...: surf = ct.Solution('ptcombust.yaml', name='Pt_surf', adjacent=[gas])

In [3]: 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=surf)
   ...: surf.add_reaction(rxn2)

In [4]: surf.forward_rate_constants[[3,-1]]
Out[4]: array([6.e+15, 6.e+19])

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.

@ischoegl
Copy link
Member Author

Sorry if I have another question in this context. Looking at units in gri30.yaml (or similarly in ptcombust.yaml), applying dimensions for length and quantity would yield mol/cm^3 for concentration units, whereas the standard concentration units for a Cantera gas phase are 1.0 kmol * m^-3. Based on that, there should be a conversion factor of 1000^n applied to A factors? There appears to be an implicit historic convention?

@speth
Copy link
Member

speth commented Jul 20, 2021

This is anticipated after Cantera/enhancements#87

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, rate_units are set when the reaction is attached to a Kinetics object, and the A factor has to be adjusted manually as there is no automatic conversion.

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 Kinetics object. The only reason it's required is because the constructor you've created here is built on top of the AnyMap infrastructure that does support alternative units, e.g.

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)

What somewhat bothers me here is that there is a circular dependency between Reaction and ReactionRate, which is not ideal.

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 Reaction and the ReactionRate are carrying the rate_units information, which shouldn't be necessary, at least in the long run where every Reaction has a ReactionRate object. But I'm not as sure what we should do in the meantime.

Beyond, it would be neat to allow for alternative unit systems when defining ReactionRate objects from scratch).

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/AnyMap input. This is, for example, already the case for setting the thermodynamic state, where all of the dedicated setters work only in numerical values in the MKS+kmol system, but the ThermoPhase.setState(AnyMap&) function will convert from any other unit system.

Incidentally, there appears to be a bug even for the YAML route [...] as the new reaction should be a duplicate of reaction 4.

Careful - if you copy the reaction specification without bringing along the corresponding units definition from the input file, the values are going to get messed up. The following works fine:

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)

Looking at units in gri30.yaml (or similarly in ptcombust.yaml), applying dimensions for length and quantity would yield mol/cm^3 for concentration units, whereas the standard concentration units for a Cantera gas phase are 1.0 kmol * m^-3. Based on that, there should be a conversion factor of 1000^n applied to A factors? There appears to be an implicit historic convention?

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 gri30.yaml are (mol, cm), as specified in the file-level units field. For a bimolecular reaction in such a phase, the rate constant has dimensions of length^3/quantity/time, which in this case means cm^3/mol/s. To convert from cm^3/mol to m^3/kmol, the conversion factor is 0.01^3/0.001 = 0.001. And you can see that this is the conversion factor being applied to the values in gri30.yaml, e.g. reaction 3 which has rate-constant: {A: 3.87e+04, b: 2.7, Ea: 6260.0} in the YAML file and once loaded gives:

>>> gri.reaction(2).rate.pre_exponential_factor
38.7

There's nothing implicit about this conversion.

@ischoegl
Copy link
Member Author

ischoegl commented Jul 20, 2021

Why is the numerical value for A different in these two examples?

You are correct - they should be the same (and different from YAML). Neither variant is aware of a UnitSystem.

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 Reaction and the ReactionRate are carrying the rate_units information, which shouldn't be necessary, at least in the long run where every Reaction has a ReactionRate object. But I'm not as sure what we should do in the meantime.

While I am anticipating to move rate_units to ReactionRate eventually, I am not sure that this will change anything about the situation. A complete resolution would require allowing for an optional units argument that takes a UnitSystem (or the equivalent dictionary) as an argument. Note that the example I am using is using a pre-existing constructor that has nothing to do with the new ReactionRate framework - my sticking point is that any user has to manually convert units if they wanted to create a reaction (rate) from scratch, which is prone to error.

Careful - if you copy the reaction specification without bringing along the corresponding units definition from the input file, the values are going to get messed up.

... makes sense! So it looks like YAML already allows for this (failed to connect the dots for the units node), but it would be simple to expand the alternative 'from-scratch' variants by adding an optional units keyword argument.

There's nothing implicit about this conversion.

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.

@speth
Copy link
Member

speth commented Jul 20, 2021

A complete resolution would require allowing for an optional units argument that takes a UnitSystem (or the equivalent dictionary) as an argument.

At least for the Python interface, the second option here would be very straightforward. All that's needed is to accept a units argument to the ElementaryReaction constructor and include it in the AnyMap that's passed to CxxNewReaction. But you do need the Kinetics if there are any conversions to be done.

@ischoegl
Copy link
Member Author

ischoegl commented Jul 20, 2021

the second option here would be very straightforward

Absolutely!

But you do need the Kinetics if there are any conversions to be done.

Yes. But it could be optional, with conversion taking place when a Reaction is attached to Kinetics? That way, the original ‘from scratch’ interface could be grandfathered in (which doesn’t have Kinetics as an argument), while also making ReactionRate more flexible.

@speth
Copy link
Member

speth commented Jul 20, 2021

Yes. But it could be optional, with conversion taking place when a Reaction is attached to Kinetics? That way, the original ‘from scratch’ interface could be grandfathered in (which doesn’t have Kinetics as an argument), while also making ReactionRate more flexible.

I think having the Kinetics object be optional if there is no unit conversion required would be okay. That just delays figuring out the rate_units, which are really only needed when serializing to a non-default unit system, and there's a specific signal that the rate_units haven't been set (factor == 0).

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 Kinetics argument in the first place.

@ischoegl
Copy link
Member Author

ischoegl commented Jul 20, 2021

However, I do not think that we should defer converting values to Cantera's unit system.

I agree to the extent possible (I don't see a way for A not being deferred).

If I'm understanding your suggestion here correctly, the following would yield two different values for the rate constant:

Once attached, they should yield the same result. So (using #1076 for rate_coeff_units):

>>> 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 rxn1 unaffected by the attachment if that is a preference. (I don't think it is)

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'))

@speth
Copy link
Member

speth commented Jul 20, 2021

I agree to the extent possible (I don't see a way for A not being deferred).

There is a perfectly robust way to avoid deferring the conversion of A: instantiate the Reaction and the ReactionRate in the context of an appropriate Kinetics object.

Once attached, they should yield the same result.

Sorry, you're right - my example was flawed and of course the two Python objects should be sharing the same underlying C++ Reaction object. I wouldn't want add_reaction to introduce a copy operation.

But I don't think we should introduce the possibility that the value returned by rxn1.rate.pre_exponential_factor might change after adding the reaction to a Kinetics object. More specifically, Reaction.rate.pre_exponential_factor should always return a value is in the MKS+kmol system, just like basically every other function in Cantera.

@ischoegl
Copy link
Member Author

ischoegl commented Jul 20, 2021

There is a perfectly robust way to avoid deferring the conversion of A: instantiate the Reaction and the ReactionRate in the context of an appropriate Kinetics object.

This is true for Reaction, but unfortunately not for ReactionRate due to their circular dependency (rate_units won't be known, so the unit system cannot be set for ReactionRate). I do, however, believe that this can be resolved without requiring Kinetics (which also is of little help to clarify units for a stand-alone ReactionRate):

  • For MKS+kmol, rate_units would eventually apply a factor of 1.0, so not having it set is inconsequential. The only apparent impact would be that rate_coeff_units won't be set; numeric values won't change.
  • For user-defined unit systems, there is always the option to return NaN until a Reaction (or corresponding ReactionRate) is attached to Kinetics. Thus, the numeric value wouldn't change dynamically (which I do agree is not great). As the user overrides the default, some consequences (which should be documented) should not be completely unexpected. As stressed above, I believe that there is value in converting units for the user as it is simply too error-prone.

The main benefit here would be that it allows for ReactionRate to be instantiated without requiring the user to already know what the corresponding Reaction looks like. Or, to put it differently, a stand-alone ReactionRate object would be logically consistent.

@speth
Copy link
Member

speth commented Jul 21, 2021

* For MKS+kmol, `rate_units` would eventually apply a factor of 1.0, so not having it set is inconsequential. The only apparent impact would be that `rate_coeff_units` won't be set; numeric values won't change.

Agreed. And for this purpose, we already have the constructors for ReactionRate classes that just take numeric values that are always treated as being in the MKS+kmol system.

* For user-defined unit systems, there is always the option to return `NaN` until a `Reaction` (or corresponding `ReactionRate`) is attached to `Kinetics`. Thus, the numeric value wouldn't change dynamically (which I do agree is not great). As the user overrides the default, some consequences (which should be documented) should not be completely unexpected. As stressed above, I believe that there is value in converting units for the user as it is simply too error-prone.

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 Arrhenius, this would mean that you have to set the member variables of the ArrheniusRate class like m_A and m_logA to NaN, and store the values whose dimensions are yet to be determined and the associated input unit system somewhere else until the reaction is added to a Kinetics object. But if you don't need to be able to evaluate the rate until the units are worked out, we already have a structure for storing such parameters - it's called AnyMap, and the fields for the ReactionRate are just a subset of the fields that are normally part of the Reaction object. At that point, you may as well just create the Reaction and ReactionRate together.

The main benefit here would be that it allows for ReactionRate to be instantiated without requiring the user to already know what the corresponding Reaction looks like. Or, to put it differently, a stand-alone ReactionRate object would be logically consistent.

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 Arrhenius class and others already work this way, if you just re-interpret A as being in arbitrary units, then the resulting rate constant has those same arbitrary units. But beyond that, I just don't see the use case -- under what circumstances would I come up with parameters for a rate constant and only later decide what reaction those are parameters for?

@ischoegl
Copy link
Member Author

@speth ... thanks for your comments.

Agreed. And for this purpose, we already have the constructors for ReactionRate classes that just take numeric values that are always treated as being in the MKS+kmol system.

👍 Ok. I believe this addresses the sticking point in #1061, where my latest update for ReactionRate.from_dict and ReactionRate.from_yaml no longer requires a Kinetics object.

[...] this would mean that you have to set the member variables of the ArrheniusRate class like m_A and m_logA to NaN, and store the values whose dimensions are yet to be determined and the associated input unit system somewhere else until the reaction is added to a Kinetics object. [...] we already have a structure for storing such parameters - it's called AnyMap, and the fields for the ReactionRate are just a subset of the fields that are normally part of the Reaction object.

This is the most logical implementation. If we agree on that, various constructors could accommodate a units keyword.

[...] I don't think a rate constant has any meaning beyond being an arbitrary mathematical expression. [...] under what circumstances would I come up with parameters for a rate constant and only later decide what reaction those are parameters for?

I agree that rate constants are by definition associated with a reaction. My concern here is mainly to break the circular dependency, and to create a fully featured Python API (including unit conversion).

Regardless of the final verdict here, I believe there's a clear path forward at this point:

@ischoegl
Copy link
Member Author

@speth ... at this point, I addressed the last open question in #1061. For the time being, units/unit systems that are not m/kmol are not allowed fow stand-alone ReactionRate objects. As rate_units will apply a factor of 1.0, I do not expect any issues with this approach.

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 a pull request may close this issue.

3 participants