-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
Add Reaction factory #745
Add Reaction factory #745
Conversation
Codecov Report
@@ 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.
|
304b569
to
61c4f20
Compare
@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 |
@decaluwe ... no problem. The main philosophy is to replace hard-coded reaction instantiation by an extendable
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. cantera/include/cantera/kinetics/InterfaceKinetics.h Lines 525 to 549 in 719cb3d
|
cd59b1d
to
3b5cfcb
Compare
After looking at this some more, I think this PR is ready for review as it stands. Other comments that go beyond this PR:
|
Eliminating |
@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++. |
ae3a7d6
to
1306ef5
Compare
@speth ... not sure whether you're considering this for 2.5, but the PR would be ready for review. |
14902ca
to
54a2013
Compare
- replace 'wrapReaction' by 'Reaction.wrap' method - eliminate hard-coded object instantiation
- deprecate magic numbers
- relocation to ReactionFactory removes function from interface
54a2013
to
510691a
Compare
@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 |
Reopened as #982 (cannot reopen here as I rebased the branch after closing the PR). |
Changes proposed in this pull request
ReactionFactory
in C++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