Introduce BulkRate class template

[ischoegl]

Changes proposed in this pull request

The clear separation of ArrheniusBase and ArrheniusRate introduced in #1184 created an opening for the creation of class templates that can be further developed in #1181. Specifically, this PR introduces BulkRate<RateType,DataType>, along with stripped-down classes Arrhenius3, TwoTempPlasma and BlowersMasel.

The pre-existing classes are now defined as typedef's as:

typedef BulkRate<Arrhenius3, ArrheniusData> ArrheniusRate;
typedef BulkRate<BlowersMasel, BlowersMaselData> BlowersMaselRate;
typedef BulkRate<TwoTempPlasma, TwoTempPlasmaData> TwoTempPlasmaRate;

Beyond, additional streamlining targets the elimination of TwoTempPlasmaReaction, as well as some updates in test_reaction.py.

It is anticipated that the definition of Arrhenius3 and BlowersMasel will facilitate the introduction of class templates InterfaceRate<> and StickingRate<> in #1181. In addition, there is a potential use of Arrhenius3 for a ThreeBodyRate<> template in a future resolution of Cantera/enhancements#133.

Also: eliminate multiple inheritance; while this allowed avoiding several virtual methods, multiple inheritance in combination with templates resulted in a small speed penalty, although the origin is inconclusive. While elimination of multiple inheritance shows a small improvement, tests on windows/MSVC indicate that #1184 did not introduce any noticeable regressions.

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 #1187 (e0095a4) into main (f85d54f) will decrease coverage by 0.03%.
The diff coverage is 76.82%.

@@            Coverage Diff             @@
##             main    #1187      +/-   ##
==========================================
- Coverage   65.44%   65.41%   -0.04%     
==========================================
  Files         318      318              
  Lines       46106    46085      -21     
  Branches    19604    19604              
==========================================
- Hits        30175    30145      -30     
+ Misses      13435    13426       -9     
- Partials     2496     2514      +18     

Impacted Files	Coverage Δ
include/cantera/kinetics/Reaction.h	100.00% <ø> (ø)
include/cantera/kinetics/ReactionRate.h	82.92% <0.00%> (-4.26%)	⬇️
include/cantera/kinetics/RxnRates.h	88.23% <0.00%> (-1.31%)	⬇️
src/kinetics/Reaction.cpp	80.89% <ø> (+0.41%)	⬆️
include/cantera/kinetics/Arrhenius.h	81.91% <58.82%> (-16.11%)	⬇️
src/kinetics/RxnRates.cpp	90.90% <92.85%> (ø)
include/cantera/kinetics/Falloff.h	77.98% <100.00%> (ø)
src/kinetics/Arrhenius.cpp	98.98% <100.00%> (+4.13%)	⬆️
src/kinetics/Falloff.cpp	84.64% <100.00%> (ø)
src/kinetics/ReactionFactory.cpp	90.04% <100.00%> (-2.03%)	⬇️
... and 8 more

---

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 f85d54f...e0095a4. Read the comment docs.

[ischoegl]

This is ready for review. The four cancelled CI runs are due to a Windows-2016 brown-out. Edit: retriggered ...

[speth]

I like the simplification that this achieves, both in complexity and lines of code, by eliminating the use of multiple inheritance with the ReactionRate class. But I think we might be able to actually go a step further and skip the BulkRate template entirely with no real loss, since it's just delegating almost all of its behavior to ArrheniusBase.

I can still see this template structure potentially being useful in the case you mentioned of interface reactions, where we need to handle some common operations on top of either the base Arrhenius or Blowers-Masel rate types. And I can see retaining BulkRate if there's some set of behaviors that we need for bulk rates that shouldn't be part of the interface rates, but I don't see anything like that in the current implementation.

[ischoegl]

@speth ... thanks for the comment! I removed the chained function calls, although I don't think there is any performance penalty. Based on your explanation, I don't see any benefit in keeping this around.

But I think we might be able to actually go a step further and skip the BulkRate template entirely with no real loss, since it's just delegating almost all of its behavior to ArrheniusBase.

There is the exception of unique_ptr<MultiRateBase> newMultiRate(), which cannot be pushed back to a lower level - we need to have this attached to a class definition that is final. So I believe BulkRate<> makes sense from this perspective. I am btw wondering whether we can construct a BulkRate<Custom, CustomData> ... .

And I can see retaining BulkRate if there's some set of behaviors that we need for bulk rates that shouldn't be part of the interface rates, but I don't see anything like that in the current implementation.

Fwiw, StickingRate<> will need a different serialization, although this is minor.

So I think it may make sense to keep BulkRate<>, at least for the time being? As long as there's no performance penalty, it would imho be a self-documenting feature as well.

[speth]

There is the exception of unique_ptr<MultiRateBase> newMultiRate(), which cannot be pushed back to a lower level - we need to have this attached to a class definition that is final. So I believe BulkRate<> makes sense from this perspective.

You're right that newMultiRate() needs to be overridden in each class, but I don't think any of the classes actually need to be declared final, since the key performance sensitive methods (xyzFromStruct) are all non-virtual in the first place.

I guess eliminating BulkRate later is an option, if it turns out not to be useful in the InterfaceKinetics implementation. Although that would I think introduce more churn of renaming what is currently the ArrheniusBase class yet another time.

[ischoegl]

I guess eliminating BulkRate later is an option, if it turns out not to be useful in the InterfaceKinetics implementation. Although that would I think introduce more churn of renaming what is currently the ArrheniusBase class yet another time.

That would be my preference. I don’t see ArrheniusBase going away either way, and the typedef’s hide BulkRate from the API already.

[ischoegl]

@speth ... fwiw, I believe that BulkRate<> can be easily leveraged to create

typedef BulkRate<CustomFunc1, ArrheniusData> CustomFunc1Rate;

by renaming getParameters to getRateParameters (mainly to raise an exception), and adding a corresponding setRateParameters (again to handle an exception). With that, I believe it would be simple to remove CustomFunc1Reaction. I'm not tackling this at the moment though as it is a tangent that I'd like to keep for later (mainly as a nomenclature has to be developed - CustomFunc1 is clearly not ideal, - and there may be ways to introduce Delegators). But again, the idea would be to have a CustomFunc1 as a simple reusable unit for class templates BulkRate<>, InterfaceRate<>, StickingRate<>, etc..

At the moment, I'll wait for this PR to be merged before getting back to #1181.

[speth]

I had just a couple of very minor comments. Otherwise this looks good to me.

[ischoegl]

@speth ... thank you for the review! I updated as requested.