Customizable data objects for ExtensibleRate

[speth]

Changes proposed in this pull request

This PR introduces ExtensibleRateData class that is used for collecting state data and being passed as the argument to the eval method of ExtensibleRate objects, analogous to the C++ implementation of reaction rates.

This is the first follow-up to #1382, and contributes to the implementation of Cantera/enhancements#79.

If applicable, provide an example illustrating new features this pull request is introducing

The implementation of an ExtensibleRate now looks like the following (from custom_reactions.py):

class ExtensibleArrheniusData(ct.ExtensibleRateData):
    __slots__ = ("T",)
    def __init__(self):
        self.T = None

    def update(self, gas):
        T = gas.T
        if self.T != T:
            self.T = T
            return True
        else:
            return False

@ct.extension(name="extensible-Arrhenius", data=ExtensibleArrheniusData)
class ExtensibleArrhenius(ct.ExtensibleRate):
    __slots__ = ("A", "b", "Ea_R")
    def set_parameters(self, params, units):
        self.A = params["A"]
        self.b = params["b"]
        self.Ea_R = params["Ea_R"]

    def eval(self, data):
        return self.A * data.T**self.b * exp(-self.Ea_R/data.T)

Checklist

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

[codecov]

Codecov Report

Merging #1417 (3ba382e) into main (760b0fb) will decrease coverage by 0.10%.
The diff coverage is 76.80%.

@@            Coverage Diff             @@
##             main    #1417      +/-   ##
==========================================
- Coverage   70.84%   70.75%   -0.10%     
==========================================
  Files         376      378       +2     
  Lines       54784    55117     +333     
  Branches    18137    18158      +21     
==========================================
+ Hits        38813    38996     +183     
- Misses      13506    13612     +106     
- Partials     2465     2509      +44     

Impacted Files	Coverage Δ
include/cantera/base/Solution.h	93.33% <ø> (ø)
include/cantera/base/ExtensionManager.h	42.85% <50.00%> (-7.15%)	⬇️
src/base/ExtensionManager.cpp	52.94% <52.94%> (ø)
src/kinetics/ReactionRateDelegator.cpp	68.75% <68.75%> (ø)
interfaces/cython/cantera/reaction.pyx	81.08% <72.72%> (+0.67%)	⬆️
src/base/Solution.cpp	77.32% <77.77%> (+0.02%)	⬆️
include/cantera/kinetics/ReactionRateDelegator.h	87.50% <80.00%> (-2.98%)	⬇️
interfaces/cython/cantera/delegator.pyx	78.60% <94.11%> (+0.59%)	⬆️
include/cantera/base/Delegator.h	66.03% <100.00%> (+1.33%)	⬆️
include/cantera/kinetics/Kinetics.h	33.57% <100.00%> (+2.97%)	⬆️
... and 36 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[speth]

I'm not sure what's going on with code coverage here. The deltas shown in Codecov seem to be all over the place for several recent PRs (for example, #1413) that should have had almost no changes, and in this case, I see lines/functions being showed as uncovered (such as ExtensionManager::wrapSolution) that are definitely called during the test suite on the coverage runner. These lines are shown as uncovered in the gcov report that's uploaded as an artifact, so it's not (just) a Codecov issue.

[ischoegl]

@speth ... thank you for your continued work on extensible objects! (and sorry about false starts for this review.) The implementation itself looks good to me, but I have some (minor?) comments, which will hopefully improve the user experience. It is to be expected that users will create buggy extensions, where improved exception handling will give valuable feedback.

On a separate note, it turns out that the simplistic CustomRate is about twice as fast as ExtensibleRate:

%run custom_reactions.py
Average time of 100 simulation runs for 'gri30.yaml' (CH4)
- New framework (YAML): 18.44 μs/step (T_final=2780.21)
- Two Custom reactions: 18.86 μs/step (T_final=2780.21) ...+2.29%
- Two Extensible reactions: 19.43 μs/step (T_final=2780.21) ...+5.37%

There is likely little that can be done about this (ExtensibleRate/ExtensibleRateData is extremely flexible and can be serialized); at the same time, I was hoping that CustomRate could be deprecated in favor of ExtensibleRate. What's your opinion here?

[speth]

There is likely little that can be done about this (ExtensibleRate/ExtensibleRateData is extremely flexible and can be serialized); at the same time, I was hoping that CustomRate could be deprecated in favor of ExtensibleRate. What's your opinion here?

In terms of performance, the sample problem doesn't tell the whole story. Both CustomRate and ExtensibleRate will call the Python rate evaluation function the same number of times, and this call count will scale with the number of custom/extensible reactions. However, CustomRate still has its function for updating the ReactionData object implemented in C++. This function is called only once per reaction type, and so this overhead won't increase any further if more rates are implemented in Python, assuming there are only a few distinct reaction types. It just so happens that for this particular case, because of the conditions where the rate data needs to (potentially) be updated, we end up with approximately twice as many calls to Python-wrapped functions as for the CustomRate route.

I would say that the decision on whether to keep CustomRate should come down to usability. For the cases where it can be used now, it is certainly less code to write than an equivalent version of ExtensibleRate. Perhaps the right compromise would be a built-in ExtensibleRate implementation for rates that are functions of temperature only.

I have a few ideas on how to improve the performance of these Python rates, but those are definitely for a later PR.

[ischoegl]

@speth ... thanks for taking care of the improved exception handling, which I believe will go a long way.

PS: I also appreciate your thoughts on performance.

[speth]

@ischoegl, can you re-approve? My fiddling around to clear up some unrelated CI issues cleared your previous approval.