Codecov Report

Merging #745 into master will increase coverage by 0.04%.
The diff coverage is 92.64%.

@@            Coverage Diff             @@
##           master     #745      +/-   ##
==========================================
+ Coverage   71.54%   71.59%   +0.04%     
==========================================
  Files         372      374       +2     
  Lines       44348    44457     +109     
==========================================
+ Hits        31730    31829      +99     
- Misses      12618    12628      +10

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 b74f11b...2c57527. Read the comment docs.

Stopping here as any additional changes would depend on #747 and #748 (and #749). PR is ready for feedback/review.

@ischoegl Could you provide a brief bit of discussion here on what the approach/philosophy is, and how it differs from what is done (or is this already discussed, elsewhere)?

I see some changes in the ElectrochemicalReaction in 76d82a3, for instance, that don't exactly make sense to me, but I'd like to get a better sense of your overall approach here, and what the current approach is, before commenting. This obviously also relates to #749, so I'd like to get a handle on your recommended approach, here.

@decaluwe ... no problem. The main philosophy is to replace hard-coded reaction instantiation by an extendable ReactionFactory approach. None of this affects calculated outcomes. I.e. I'm just repackaging what's already there, with the difference that it becomes very simple to add additional reaction types.

• GasKinetics is one case in point: it is now fully flexible, i.e. someone can define a new reaction type externally and link it against cantera (ctwrap approach, where new C++ lambda functions are registered for an existing cantera factory object). Same could be done for InterfaceKinetics & co. (although the existing code is more convoluted / harder to untangle).
• Approach:
  • Replace magic number referenced function calls by name-mapped C++ lambda functions calls (which are mainly used for setup; actual kinetics calculations involve templated functions, i.e. there is no change from the current implementation whatsoever)
  • I ended up using C++ lambda functions to recycle existing code directly (instead of the first commit that used new virtual member functions)
  • Ideally, the reaction list should be non-hierarchical (i.e. no nested definitions like falloff)
• The three things I came across are now YAML: consistently clarify reaction type #747, Refactor implementation of falloff reactions #748, and Discuss deprecating Butler-Volmer reaction types #749, where a minor update to the existing YAML syntax could simplify things quite a bit (i.e. making the implementation simpler to maintain/understand and also more future-proof regarding possible extensions)

PS: any existing XML files for additional electrochemical reaction definitions would be beneficial. From what I can tell, there are at least three different behaviors implemented (and accessible via XML), some of which are currently not reachable via YAML.

After looking at this some more, I think this PR is ready for review as it stands.

Other comments that go beyond this PR:

• If the general direction is ok, a next step would be Discuss deprecation of chemically-activated reaction type #751. (It warrants discussion beyond this PR.)
• I no longer believe that a completely flat structure (I left some comments to that extent) brings significant benefits. E.g. it's obviously ok to have a falloff type with three variants/flavors that are based on a reaction rate type (i.e. Lindemann, Troe and SRI), so Refactor implementation of falloff reactions #748 is moot (and closed).
• The same type + variant approach could be used for electrochemical and butler-volmer flavors down the line (see Discuss deprecating Butler-Volmer reaction types #749/Deprecate undocumented Butler-Volmer reaction options #750)
• One question I have is: Is there a good reason to implement a FalloffFactory? (it appears to be highly specialized.) I can see the point of factory classes to break things out for Python access, but it may make more sense to do this at a more generic RxnRatesFactory level?

One question I have is: Is there a good reason to implement a FalloffFactory? (it appears to be highly specialized.) I can see the point of factory classes to break things out for Python access, but it may make more sense to do this at a more generic RxnRatesFactory level?

Eliminating FalloffFactory sounds plausible to me. I doubt anyone is going to come up with an alternative falloff function without wanting more flexibility than this functional form provides. Making the reaction rate functions properly generic and more customizable is a bit more involved, given that there are at least perceived performance reasons for the less flexible way things are currently implemented using the Rate1 template class.

@speth ... thanks for the comment: this is what I thought. I am aware of speed benefits of templates used for reaction rates (the dynamic classes appear to be used almost exclusively for setup).

Will most certainly not include anything else in this PR: I'd like to untangle the Python front end first (which is done), before anything else can get resolved in C++.

@speth ... not sure whether you're considering this for 2.5, but the PR would be ready for review.

@speth ... not sure whether this has legs or not (there may be other considerations that I’m not aware of). If this is a dead end, I’ll simply close this.

Closing as this has been open and pending review for more than 6 months.

A rebased branch remains on my fork on ischoegl:reaction-factories

Reopened as #982 (cannot reopen here as I rebased the branch after closing the PR).