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
Update ReactionData and consolidate update strategies #1153
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1153 +/- ##
==========================================
- Coverage 66.04% 66.03% -0.02%
==========================================
Files 313 313
Lines 44939 44901 -38
Branches 19107 19108 +1
==========================================
- Hits 29679 29649 -30
+ Misses 12678 12673 -5
+ Partials 2582 2579 -3
Continue to review full report at Codecov.
|
c2fe0b6
to
26d62ac
Compare
@speth ... these are some changes that are a preamble to #1089. While the |
26d62ac
to
ac0c875
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.
@speth ... I know you're somewhat luke-warm on the ability to evaluate rate evaluators without having them attached to a Reaction
object. I think removing these quirky setters may alleviate some of the concerns.
Some of your contribution in #1101 and #1151 made the old 'update' mechanism pretty much obsolete, as a ReactionRate
evaluator always has a ReactionData
object available. I already removed part of the updateFromStruct
mechanism; this would be a logical conclusion.
24dd022
to
dfabbc6
Compare
dfabbc6
to
9f74f1d
Compare
@speth ... based on your prompt about the status of this PR, I had another look at what simplifications can be taken care of prior to #1089. It turns out that your ideas from #1101 and #1151 made a couple of other simplifications possible.
|
50466c5
to
f8b4f0a
Compare
There was one additional simplification that I left on the table (taken care of now), which also made Python's |
Splitting GasKinetics::updateROP into parts allows for code portions to be reused by other methods. Three internal methods are introduced: * GasKinetics::processFwdRateCoefficients * GasKinetics::processThirdBodies * GasKinetics::processEquilibriumConstants As the methods are defined in the header only, there is no speed penalty.
f8b4f0a
to
1443057
Compare
@BangShiuh ... rebased and updated docstrings for the update mechanism (clarifying what is used when). Hope this helps with #1099. |
1443057
to
4144bff
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.
This looks great, @ischoegl. I just had a few very minor comments to be considered.
6edea53
to
285ce29
Compare
@speth ... thanks for the prompt review! |
285ce29
to
a420d3d
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.
Looks good to me. Thank you, @ischoegl!
Changes proposed in this pull request
ReactionData
updateFromStruct
GasKinetics::updateROP
into multiple segmentsFalloffReaction::thirdBodyConcentration
andBlowersMasel::deltaH
byReactionData
avenueddT
from rate objects (to be superseded by jacobian evaluators in Sparse Jacobians for GasKinetics #1089)The PR further expands on ideas introduced in #1151
If applicable, provide an example illustrating new features this pull request is introducing
With this PR:
compared to current
main
:I ran the speed tests several times; there appears to be no noticeable impact of the code reorganization on performance. At least on my machine, run-to-run variations are within 1-2 ms, so things are overall consistent.
Checklist
scons build
&scons test
) and unit tests address code coverage