Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make reactions extendable #982

Merged
merged 67 commits into from
Mar 19, 2021
Merged

Make reactions extendable #982

merged 67 commits into from
Mar 19, 2021

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Feb 20, 2021

Changes proposed in this pull request

  • Add ReactionFactory in C++
  • Replace hard-coded instantiation and setup in C++ and Python
  • Remove magic numbers from Python interface
  • A new abstract base class ReactionRateBase allows for flexible implementations (derived classes are de-virtualized)
  • A new MultiRateBase class is introduced as an alternative to Rate1 templates
  • A new CustomReaction in Python uses Func1 objects

Approach

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

Example: GasKinetics becomes 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).

Implementation

  • 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).
  • C++ lambda functions recycle existing code directly

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

Supersedes #745 (can no longer be opened as it was rebased)

Fixes Cantera/enhancements#63 (a new framework is introduced, but not fully implementated)

A full replacement of Rate1 goes beyond the scope of this PR.

References:

Checklist

  • There is a clear use-case for this code change
  • The commit message has a short title & references relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • The pull request is ready for review

@codecov
Copy link

codecov bot commented Feb 21, 2021

Codecov Report

Merging #982 (4fe058f) into main (d557e50) will decrease coverage by 0.89%.
The diff coverage is 86.24%.

❗ Current head 4fe058f differs from pull request most recent head b770450. Consider uploading reports for the commit b770450 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #982      +/-   ##
==========================================
- Coverage   71.56%   70.67%   -0.90%     
==========================================
  Files         359      362       +3     
  Lines       45666    44555    -1111     
==========================================
- Hits        32683    31489    -1194     
- Misses      12983    13066      +83     
Impacted Files Coverage Δ
include/cantera/kinetics/Kinetics.h 51.51% <ø> (ø)
include/cantera/kinetics/RxnRates.h 92.13% <ø> (-0.34%) ⬇️
src/kinetics/importKinetics.cpp 97.60% <ø> (ø)
src/zeroD/Reactor.cpp 85.18% <ø> (+0.09%) ⬆️
test/kinetics/kineticsFromYaml.cpp 100.00% <ø> (ø)
include/cantera/kinetics/ReactionRate.h 40.74% <40.74%> (ø)
include/cantera/kinetics/MultiRate.h 45.45% <45.45%> (ø)
include/cantera/kinetics/ReactionData.h 69.23% <69.23%> (ø)
src/kinetics/BulkKinetics.cpp 88.50% <72.72%> (-5.35%) ⬇️
include/cantera/kinetics/ReactionFactory.h 80.95% <80.95%> (ø)
... and 229 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 d557e50...b770450. Read the comment docs.

@ischoegl ischoegl marked this pull request as draft February 22, 2021 16:01
@ischoegl
Copy link
Member Author

ischoegl commented Feb 22, 2021

kicking this back to draft - I am planning to extend the framework by a custom Python class to work out some of the issues mentioned in Cantera/enhancements#63 (there are some bits in GasKinetics.h that need to be worked out).

@ischoegl ischoegl changed the title Reaction factories Make reactions extendable Feb 22, 2021
@ischoegl ischoegl requested a review from speth March 15, 2021 03:26
@ischoegl ischoegl marked this pull request as ready for review March 15, 2021 11:54
* streamline names:
  - RxnRate -> ReactionRate
  - CustomPyRate -> CustomFunc1Rate
  - CustomPyReaction -> CustomFunc1Reaction
* label some (potentially temporary) methods as @internal
* minor fixes in Python interface
After removal of XML support, some service functions can be removed to
simplify the code structure.
The member 'setup' method avoids having to recreate separate stand-alone
setup methods.
The intermediate class prevents naming clashes between derived classes
(e.g. 'rate' members).
@ischoegl ischoegl force-pushed the reaction-factories branch from fcb5a58 to b3194f6 Compare March 15, 2021 13:08
@ischoegl
Copy link
Member Author

ischoegl commented Mar 15, 2021

Force-pushed as I found a better solution to allow for Reaction2::rate/Reaction2::setRate getters/setters (the intermediate object Reaction2 can be eliminated once the new framework is fully adopted and old objects are deprecated). At this point I'll stop for comments.

PS: As of this morning, it looks like boost is - at least partially - broken (#990)

PPS: Overall, I'd propose to limit the current PR to the creation of the MultiRateBase framework (which should be more or less done - I'll finish doc strings once things look like they are converged). I assume once everything is updated this will end up around 2k lines. Subsequent PR's can transition existing reaction objects to the new framework (I envision 2 PR's for remaining BulkKinetics, - one for everything except Falloff, and one for Falloff, - and eventually a third PR for InterfaceKinetics). Breaking this up should also help to limit conflicts with other ongoing efforts (blowers-masel, electrochemistry, etc.).

Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @ischoegl, I think this is getting very close. Most of my comments below are fairly minor implementation details. I have no problem leaving the follow-on work you've mentioned to later PRs. The only thing I'm not looking forward to is fixing the merge conflicts this will introduce to #984 😬.

One thing I'm still a little iffy on is all the constructors that have to be implemented for each DataType class in order to support evaluation of rates for reactions not associated with any Kinetics object. While I recognize that T and P are sufficient for quite a few cases, what are you supposed to do for the cases where that doesn't work? Having to implement all of those constructors and throw an exception in them seems kind of tedious, and I don't think we should require users to do that. Would it make sense to have a common base class for the DataType classes, with update_T and update_TP methods that throw NotImplementedError by default?

src/kinetics/ReactionRate.cpp Outdated Show resolved Hide resolved
include/cantera/kinetics/ReactionRate.h Show resolved Hide resolved
include/cantera/kinetics/MultiRate.h Outdated Show resolved Hide resolved
include/cantera/kinetics/MultiRate.h Outdated Show resolved Hide resolved
include/cantera/kinetics/ReactionRate.h Outdated Show resolved Hide resolved
include/cantera/kinetics/ReactionRate.h Outdated Show resolved Hide resolved
interfaces/cython/cantera/reaction.pyx Outdated Show resolved Hide resolved
src/kinetics/BulkKinetics.cpp Outdated Show resolved Hide resolved
src/kinetics/ReactionFactory.cpp Outdated Show resolved Hide resolved
src/kinetics/ReactionRate.cpp Outdated Show resolved Hide resolved
@ischoegl
Copy link
Member Author

ischoegl commented Mar 19, 2021

@speth ... done. I actually looked at #984 and there are surprisingly few (and simple) merge conflicts. I ended up renaming Reaction2::setup to Reaction2::setParameters to mirror getParameters you are defining there (also moved it to Reaction2 as I don't have plans to backport that functionality).

I also looked into your comment on DataType. I tried a base class, but couldn't really find much in terms of simplifications. I did, however, implement multiple update versions, which ended up making things somewhat simpler to follow. I also moved all of the data types to ReactionData.h/.cpp for clarity. Docstrings are updated as well - I'll add more for the follow-up PR's. There may be some additional minor items, but otherwise I believe it's ready to be merged.

Regarding next steps, I may wait for #984 to be merged, as there are some synergies. Once that is done, I am planning to create new classes mirroring the old ones (e.g. ElementaryReaction2, etc.) so things can be cleanly deprecated.

Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick revisions. This all looks good to me.

I think these changes to the ReactionData interface will work fine, at least for the time being. I imagine that as we exercise this capability on the existing rate classes, we'll be able to get a better feel for possible improvements / simplifications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make reactions extendable
2 participants