-
-
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
Refactor reaction rate evaluators #995
Conversation
Codecov Report
@@ Coverage Diff @@
## main #995 +/- ##
==========================================
+ Coverage 72.59% 72.92% +0.33%
==========================================
Files 356 357 +1
Lines 46508 47120 +612
==========================================
+ Hits 33763 34363 +600
- Misses 12745 12757 +12
Continue to review full report at Codecov.
|
8f1f036
to
cccc20f
Compare
fdb0749
to
ffc1b04
Compare
3e7a7ed
to
4d77a68
Compare
@speth ... I think this is getting close to where it should be. Benchmarks (YAML vs XML) are:
So performance comparisons are still in line with #982. There are a couple of items I am aware of but would rather tackle in combination with
PS: |
4d77a68
to
1eca0d8
Compare
One wart of the implementation thus far was that
|
0875a2b
to
c598603
Compare
@speth ... I believe this is now (finally) converged after deciding to incorporate the ability to modify reaction rate parameters in memory. At 3.5k lines, I believe there is a stopping point ... PS: while CI passes, there is some glitch for the |
6015710
to
2665490
Compare
Also: * Remove unnecessary calls to default constructors * Update check for not-a-number * Make ReactionRate::getParameters protected * Simplify Reaction::getParameters and various constructors * Update includes to fix docs build
Also re-insert warnings about experimental custom reactions.
e311bde
to
449fe01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found just a few minor issues with the documentation. Otherwise, I think this should be finished. I'm 100% in agreement that we should address some of these secondary questions in a separate PR.
Also, I triggered a re-run of the CI to see if the failure while downloading packages was just a transient issue.
This commit squashes several approaches for the creation of a Reaction from a dictionary that corresponds to a YAML definition.
449fe01
to
952d113
Compare
@speth - thanks for catching those. Fixed the docstrings and squashed the last couple of commits as they just reshuffled approaches for a single feature. |
Thanks, @speth! |
Changes proposed in this pull request
Use newly introduced
MultiBulkRates
rate evaluator to handle replacement classes, and deprecate current approaches. This PR will replace everything exceptFalloffReaction
and introduce the following new classes:ElementaryReaction3
ThreeBodyReaction3
PlogReaction3
ChebyshevReaction3
These classes will be used by default by YAML importers, although the old classes remain accessible from YAML. CTI and XML file import will use the old classes. The
3
will be dropped from class names after 2.6, and old class names will have2
added (Edit: likely the old classes will be removed).Items that will be addressed in subsequent PRs are:
ReactionRate
and templatedMultiRate
objects.FalloffReaction
sThirdBody
handlersInterfaceKinetics
If applicable, fill in the issue number this pull request is fixing
Addresses Cantera/enhancements#84 and Cantera/enhancements#87
Checklist
scons build
&scons test
) and unit tests address code coverage