Skip to content

Make reactions extendable#982

Merged
speth merged 67 commits intoCantera:mainfrom
ischoegl:reaction-factories
Mar 19, 2021
Merged

Make reactions extendable#982
speth merged 67 commits intoCantera:mainfrom
ischoegl:reaction-factories

Conversation

@ischoegl
Copy link
Copy Markdown
Member

@ischoegl ischoegl commented Feb 20, 2021 •
edited
Loading

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

@ischoegl ischoegl mentioned this pull request Feb 20, 2021
Closed
@ischoegl ischoegl marked this pull request as draft February 21, 2021 01:30
@ischoegl ischoegl closed this Feb 21, 2021
@ischoegl ischoegl reopened this Feb 21, 2021
@ischoegl ischoegl mentioned this pull request Feb 21, 2021
Open
@ischoegl ischoegl marked this pull request as ready for review February 21, 2021 18:55
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 21, 2021 •
edited
Loading

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 mentioned this pull request Feb 21, 2021
Closed
Ingmar Schoegl and others added 10 commits February 22, 2021 07:22
@ischoegl ischoegl marked this pull request as draft February 22, 2021 16:01
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Feb 22, 2021 •
edited
Loading

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
ischoegl added 9 commits March 15, 2021 08:07
@ischoegl
[Kinetics] address 'simple' review comments for new reaction framework
7622fda 
* streamline names:
  - RxnRate -> ReactionRate
  - CustomPyRate -> CustomFunc1Rate
  - CustomPyReaction -> CustomFunc1Reaction
* label some (potentially temporary) methods as @internal
* minor fixes in Python interface
@ischoegl
[Kinetics] introduce reaction-rate specific shared data types
4d086a6
@ischoegl
[Kinetics] create ArrheniusRate from AnyMap
efbe7bc
@ischoegl
[Kinetics] add XML related 'todo' comments to ReactionFactory
7362930 
After removal of XML support, some service functions can be removed to
simplify the code structure.
@ischoegl
[Kinetics] implement Reaction::setup method
0454bf4 
The member 'setup' method avoids having to recreate separate stand-alone
setup methods.
@ischoegl
[Kinetics] add MultiRateBase evaluator class
aa4d072
@ischoegl
[Kinetics] switch to MultiRateBase framework
1b15047
@ischoegl
[Kinetics] minor updates
d046a0f
@ischoegl
[Kinetics] add intermediate class Reaction2 as base for new framework
b3194f6 
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
Copy Markdown
Member Author

ischoegl commented Mar 15, 2021 •
edited
Loading

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

@ischoegl ischoegl mentioned this pull request Mar 15, 2021
Open
speth
speth requested changes Mar 18, 2021
View reviewed changes
Copy link
Copy Markdown
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?

@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Mar 19, 2021 •
edited
Loading

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

speth
speth approved these changes Mar 19, 2021
View reviewed changes
Copy link
Copy Markdown
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.

@speth speth merged commit bbb752a into Cantera:main Mar 19, 2021
This was referenced Mar 19, 2021
Closed
Merged
@bryanwweber bryanwweber mentioned this pull request Mar 25, 2021
Closed
@ischoegl ischoegl mentioned this pull request Mar 25, 2021
Merged
8 tasks
@speth speth mentioned this pull request Jun 15, 2021
Merged
8 tasks
@ischoegl ischoegl deleted the reaction-factories branch October 17, 2021 02:39
@speth speth mentioned this pull request Nov 29, 2021
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@speth speth speth approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make reactions extendable

2 participants

@ischoegl @speth