Refactor electrochemical reactions

[ischoegl]

Changes proposed in this pull request

• Add ability to evaluate charge-transfer reactions to existing InterfaceRate<> and StickingRate<> templates
• The existing test suite (specifically test_kinetics.py::TestSofcKinetics) covers all changes in detail

If applicable, fill in the issue number this pull request is fixing

Closes Cantera/enhancements#139, addresses Cantera/enhancements#142

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

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 #1216 (7a1f478) into main (e6a5ffd) will increase coverage by 0.01%.
The diff coverage is 83.67%.

@@            Coverage Diff             @@
##             main    #1216      +/-   ##
==========================================
+ Coverage   65.42%   65.44%   +0.01%     
==========================================
  Files         320      320              
  Lines       46247    46321      +74     
  Branches    19655    19688      +33     
==========================================
+ Hits        30259    30315      +56     
- Misses      13460    13475      +15     
- Partials     2528     2531       +3     

Impacted Files	Coverage Δ
include/cantera/base/utilities.h	96.42% <ø> (ø)
include/cantera/kinetics/InterfaceKinetics.h	85.71% <ø> (ø)
include/cantera/kinetics/MultiRateBase.h	100.00% <ø> (ø)
src/kinetics/Kinetics.cpp	68.33% <0.00%> (-0.48%)	⬇️
include/cantera/kinetics/ReactionData.h	84.21% <30.76%> (-4.68%)	⬇️
src/kinetics/InterfaceKinetics.cpp	72.23% <70.83%> (-3.56%)	⬇️
src/kinetics/ReactionData.cpp	86.22% <80.00%> (+0.58%)	⬆️
include/cantera/kinetics/InterfaceRate.h	85.53% <82.75%> (+1.38%)	⬆️
src/kinetics/InterfaceRate.cpp	92.48% <94.36%> (+0.66%)	⬆️
include/cantera/kinetics/Arrhenius.h	82.47% <100.00%> (ø)
... and 16 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[ischoegl]

@speth / @decaluwe ... this is ready for review.

[ischoegl]

PS: also referencing Cantera/enhancements#38. FWIW, if we can rule out electrochemical 'sticking' formulations (which I currently left in), it would be very easy to introduce a third ElectrochemicalRate<> template (in analogy to the already existing InterfaceRate<> and StickingRate<> templates).

[speth]

Thanks, @ischoegl! This looks really good. I'm encouraged by seeing all of the deprecated methods in InterfaceKinetics, as a sign of how much simplification all of this refactoring will eventually bring there.

Beyond the few minor comments below, I had one other thought which is that CoverageData and CoverageBase have grown to encompass quite a bit more than coverage dependence for the rate. If there's a good reason for not breaking the electrochemical specialization out into a derived class, I'm wondering if there are at least more descriptive names for these classes that we could use.

[ischoegl]

@speth .. thank you for the prompt review! Before going over the PR, I wanted to respond to your larger question.

I had one other thought which is that CoverageData and CoverageBase have grown to encompass quite a bit more than coverage dependence for the rate. If there's a good reason for not breaking the electrochemical specialization out into a derived class, I'm wondering if there are at least more descriptive names for these classes that we could use.

I was struggling with this also. The single-most important reason for not creating a separate specialization was that I didn't want to block electrochemical 'sticking' reactions. If we can rule this out, then a ElectrochemicalRate<> template would be the way to go (but I am wary of the odd-ball mechanism that may want to use this formulation, and @decaluwe - in a brief comment - opined that it's technically possible to go that route despite having some serious misgivings).

I agree that CoverageBase may not be the best descriptor any longer. InterfaceData and InterfaceBase would be the logical nomenclature, even if it the latter sounds somewhat clunky? Suggestions welcome ... 😄

[speth]

I agree that CoverageBase may not be the best descriptor any longer. InterfaceData and InterfaceBase would be the logical nomenclature, even if it the latter sounds somewhat clunky? Suggestions welcome ... 😄

What about InterfaceRateBase in the latter case, so it doesn't sound like it's a base class for Interface, which is something else.

[ischoegl]

@speth ... done!

I'm encouraged by seeing all of the deprecated methods in InterfaceKinetics, as a sign of how much simplification all of this refactoring will eventually bring there.

It was actually surprising to see that the implementation is actually not that complex - it was just scattered all over and hard to follow.

[speth]

Thanks again, @ischoegl - this all looks good to me.