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     

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.

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

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

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.

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.

@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 ... thank you for the review! I updated as requested.