Fix bug with electrochemical-legacy

[dschmider-HSOG]

Changes proposed in this pull request

• Change alias in ReactionFactory.cpp to enable correct legacy behavior for electrochemical reactions
• Fix problem described in Cantera Users' Group

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 #1256 (a955dfd) into main (a394423) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1256      +/-   ##
==========================================
+ Coverage   65.50%   65.54%   +0.04%     
==========================================
  Files         329      329              
  Lines       46666    46668       +2     
  Branches    19855    19855              
==========================================
+ Hits        30568    30590      +22     
+ Misses      13538    13516      -22     
- Partials     2560     2562       +2     

Impacted Files	Coverage Δ
src/kinetics/ReactionFactory.cpp	96.03% <100.00%> (+2.38%)	⬆️
src/kinetics/InterfaceKinetics.cpp	72.46% <0.00%> (+0.22%)	⬆️
src/kinetics/Reaction.cpp	81.72% <0.00%> (+0.55%)	⬆️
src/thermo/SurfPhase.cpp	80.36% <0.00%> (+1.92%)	⬆️
src/kinetics/importKinetics.cpp	79.36% <0.00%> (+4.76%)	⬆️

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

[speth]

I think this change makes sense, but I'm a little concerned about what it says about how we're testing this reaction type to verify that the "legacy" and new implementations corresponding to CTI and YAML input are actually the same. If the test suite didn't catch this error, I think we need to take another look at those tests.

[ischoegl]

Thanks, @dschmider-HSOG for tracking that bug down! This was a clear oversight that happened to fall through the cracks of the test suite.

@speth Regarding unit test coverage, both electrochemical and electrochemical-legacy were already covered from the YAML side. I didn't realize that we still had a version for sofc.cti in data/inputs, which I am now also checking against. The pre-existing test is thorough, so I didn't feel it to be necessary to add things to test_reaction.py.

To facilitate a resolution here, I pushed an update to include testing of CTI input to this PR.

[speth]

Thanks for adding this test, @ischoegl. The thing that I'm still surprised to see is that if you revert the change that fixes the reaction type, this new test still passes.

One difference that I did notice between @dschmider-HSOG's example posted on the Users' Group and sofc.cti is the use of the exchangecurrentdensity option in the former case:

surface_reaction("Li+[electrolyte] + V[NCA] + electron <=> Li[NCA]",
                 [2.62895E+10, 0.0, (61.0098120252686, 'kJ/mol')],
                 rate_coeff_type = "exchangecurrentdensity",
                 beta = 0.5,
                 id="NCA_reaction")

I'm thinking we don't actually have a test that covers this case for the "legacy" reaction framework / CTI input, though the feature is used in data/inputs/lithium_ion_battery.cti.

[ischoegl]

I'm thinking we don't actually have a test that covers this case for the "legacy" reaction framework / CTI input, though the feature is used in data/inputs/lithium_ion_battery.cti.

Thanks, @speth. When refactoring, I relied on existing tests. From the implementation side, the exchangecurrentdensity is a relatively small portion where I actually was able to avoid major refactoring, as most of what is required is to set InterfaceRateBase::m_exchangeCurrentDensityFormulation (none of the implementation changed even if the 'plumbing' was reshuffled).

If there wasn't a test covering this behavior from the get-go this is a different issue. It's outside of my wheel house and should probably be added by a separate issue? Tagging @decaluwe here ...

[ischoegl]

@speth ... I opened #1259 which should resolve a larger issue. I don't think this should be added to this PR as this goes well beyond the scope of what @dschmider-HSOG signed up for.

[ischoegl]

Based on #1263, I'd suggest to merge here (fwiw, the new sofc.cti test does add coverage, while not covering the actual issue).

[speth]

Thanks for identifying this fix and submitting the PR, @dschmider-HSOG.