Codecov Report

Merging #1061 (39d4577) into main (3dc940d) will increase coverage by 0.51%.
The diff coverage is 92.55%.

@@            Coverage Diff             @@
##             main    #1061      +/-   ##
==========================================
+ Coverage   72.92%   73.44%   +0.51%     
==========================================
  Files         357      364       +7     
  Lines       47120    47595     +475     
==========================================
+ Hits        34363    34955     +592     
+ Misses      12757    12640     -117     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3dc940d...39d4577. Read the comment docs.

We can avoid the need for the MultiRateFactory class by adding a member function to the ReactionRate class, as suggested in my original outline for the MultiRate structure in #982. Namely, for each type of ReactionRate, all that's needed is a function:

virtual shared_ptr<MultiRateBase> newMultiRate() const {
    return make_shared<MultiBulkRate<ArrheniusRate, ArrheniusData>>();
}

Then, the replacement else-if tree in addReaction would work by just calling rate->newMultiRate().

@speth ... thanks for the suggestion - this is indeed simpler.

@speth and @bryanwweber - thank you for your reviews ... I addressed / responded to all comments at this point.

Regarding the unit controversy, I believe that there are some inconsistencies that go beyond this PR (see new issue #1071) - the constructor in question is useful for the creation of Reaction objects from scratch, where unit handling is not clear.

Thanks, @bryanwweber!

@speth ... I believe I resolved the issue with units by removing the Kinetics parameter. Regarding the question about where the creation of a stand-alone ReactionRate object could be useful, I believe it is critical for the creation of a Reaction object from scratch, e.g.

In [1]: import cantera as ct
   ...: 
   ...: reactants = {'O': 1., 'H2': 1}
   ...: products = {'H': 1., 'OH': 1.}
   ...: rxn = ct.ElementaryReaction(reactants, products)
   ...: rxn
   ...: 
Out[1]: <ElementaryReaction: H2 + O <=> H + OH>

In [2]: rxn.rate # reaction rate is not set up (yet)
Out[2]: <ArrheniusRate at 7f38eb99ef00>

In [3]: rxn.rate(300.)
Out[3]: nan

In [4]: rxn.rate = ct.ReactionRate.from_yaml("rate-constant: {A: 38.7, b: 2.7, Ea: 6260.0 cal/mol}")
   ...: rxn.rate
   ...: 
Out[4]: <ArrheniusRate at 7f38eb99ef60>

In [5]: rxn.rate(300.)
Out[5]: 5195.44737317942

@speth ... the last commit blocks an edge case where a user would specify a unit system that would produce rate_units that have a factor other than 1.0 (which is probably the main take-away of the discussion in #1071):

In [2]: rxn.rate = ct.ReactionRate.from_yaml("""
   ...: rate-constant: {A: 38.7, b: 2.7, Ea: 6260.0 cal/mol}
   ...: units: {length: cm, quantity: mol}
   ...: """)

[...]

CanteraError: 
*******************************************************************************
InputFileError thrown by ReactionRateFactory::newReactionRate:
Error on line 2 of input string:
Alternative units for 'length' or 'quantity` are not supported when creating
a standalone 'ReactionRate' object.
|  Line |
|     1 | 
|     2 | rate-constant: {A: 38.7 kmol/cm^3, b: 2.7, Ea: 6260.0 cal/mol}
>     3 > units: {length: cm, quantity: mol}
                 ^
*******************************************************************************

With this update, I believe that all comments are taken care of.

PS: there is an exception though:

ct.ReactionRate.from_yaml("rate-constant: {A: 4.0e+21 cm^6/mol^2/s, b: 0.0, Ea: 1207.72688}")

which is a bit harder to catch (see units-custom.yaml).

Not too hard after all. I believe this is done now ...

In [2]: rate = ct.ReactionRate.from_yaml("rate-constant: {A: 4.0e+21 cm^6/mol^2/s, b: 0.0, Ea: 1207.72688}")
---------------------------------------------------------------------------
CanteraError                              Traceback (most recent call last)
<ipython-input-2-d77e48358ab6> in <module>()
----> 1 rate = ct.ReactionRate.from_yaml("rate-constant: {A: 4.0e+21 cm^6/mol^2/s, b: 0.0, Ea: 1207.72688}")

/src/interfaces/cython/cantera/reaction.pyx in cantera._cantera.ReactionRate.from_yaml()
    109         cdef CxxAnyMap any_map
    110         any_map = AnyMapFromYamlString(stringify(text))
--> 111         cxx_rate = CxxNewReactionRate(any_map)
    112         return ReactionRate.wrap(cxx_rate)
    113 

CanteraError: 
*******************************************************************************
InputFileError thrown by Arrhenius::setParameters:
Error on line 1 of input string:
Specification of units is not supported for pre-exponential factor when
creating a standalone 'ReactionRate' object.
|  Line |
>     1 > rate-constant: {A: 4.0e+21 cm^6/mol^2/s, b: 0.0, Ea: 1207.72688}
                                   ^
*******************************************************************************

@speth ... thank you for your re-review and the interesting comments. I ended up adopting all your suggested changes.

[...] one of the key benefits of the generic ReactionRate class is breaking this requirement that there is a specific rate parameterization that goes along with a given Reaction class.

I honestly was still thinking along the old lines too much here. I certainly realized that the revised Reaction3 specializations were pretty much hollowed out (i.e. not many differences), but following that thought it will become possible to at least partially ditch having specialized Reaction classes altogether. As an example, there's nothing wrong with an elementary reaction having a Chebyshev rate object. This opens up a lot of interesting possibilities and simplifications ...

Realized that shifting the emphasis away from Reaction (using consistent naming) makes specialized getParameters mostly unnecessary (except for ThreeBodyReaction3, at least for the moment). Which means that ElementaryReaction3, PlogReaction3 and ChebyshevReaction3 are essentially the same (except for minor differences in constructors). Not bad ... 🎉

I believe this is ready to merge now. Getting rid of some Reaction specializations (while possible) is likely a longer discussion for another PR.

PS: Changing the type string output created a minor paper cut, which is also fixed.

@speth ... thanks for the reviews! I removed the redundant returns with the last commit.

Regarding the fate of the Reaction class, I expect that the only distinction ultimately worth keeping will be between bulk phase reactions and interface/electrochemical reactions. But even then, some of the extra parameters and functionality may fit better within the ReactionRate classes.

👍 ... I see it the same way - the YAML type field will essentially be pushed through to the ReactionRate object, which should be able to take care of (almost) everything.