Update ReactionData and consolidate update strategies by ischoegl · Pull Request #1153 · Cantera/cantera

ischoegl · 2021-12-06T00:37:48Z

Changes proposed in this pull request

implement inheritance for ReactionData
remove updateFromStruct
split GasKinetics::updateROP into multiple segments
some preparatory work for a rebase of Sparse Jacobians for GasKinetics #1089
replace buffer strategy for FalloffReaction::thirdBodyConcentration and BlowersMasel::deltaH by ReactionData avenue
remove service methods that are no longer necessary
remove access to ddT 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:

$ PYTHONPATH=build/python python interfaces/cython/cantera/examples/kinetics/custom_reactions.py
Average time of 100 simulation runs for 'gri30.yaml' (CH4)
- New framework (YAML): 109.55 ms (T_final=2736.60)
- One Python reaction: 118.21 ms (T_final=2736.60) ... +7.90%
- Old framework (XML): 116.51 ms (T_final=2736.60) ... +6.35%

compared to current main:

$ PYTHONPATH=build/python python interfaces/cython/cantera/examples/kinetics/custom_reactions.py
Average time of 100 simulation runs for 'gri30.yaml' (CH4)
- New framework (YAML): 109.40 ms (T_final=2736.60)
- One Python reaction: 117.50 ms (T_final=2736.60) ... +7.41%
- Old framework (XML): 117.32 ms (T_final=2736.60) ... +7.24%

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

The pull request includes a clear description of this code change
Commit messages have short titles and reference relevant issues
Build passes (scons build & scons test) and unit tests address code coverage
Style & formatting of contributed code follows contributing guidelines
The pull request is ready for review

codecov · 2021-12-06T00:57:34Z

Codecov Report

Merging #1153 (a420d3d) into main (c7c2082) will decrease coverage by 0.01%.
The diff coverage is 88.65%.

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

Impacted Files	Coverage Δ
include/cantera/kinetics/Kinetics.h	`54.87% <0.00%> (-1.38%)`	⬇️
include/cantera/kinetics/MultiRateBase.h	`100.00% <ø> (ø)`
src/kinetics/Arrhenius.cpp	`96.29% <ø> (-0.04%)`	⬇️
include/cantera/kinetics/Falloff.h	`77.98% <62.50%> (-0.78%)`	⬇️
include/cantera/kinetics/Arrhenius.h	`97.10% <77.77%> (+1.04%)`	⬆️
src/kinetics/ReactionData.cpp	`86.84% <86.79%> (-13.16%)`	⬇️
include/cantera/kinetics/RxnRates.h	`77.51% <87.50%> (-0.14%)`	⬇️
include/cantera/kinetics/Custom.h	`85.71% <100.00%> (ø)`
include/cantera/kinetics/GasKinetics.h	`100.00% <100.00%> (ø)`
include/cantera/kinetics/MultiRate.h	`71.05% <100.00%> (+5.42%)`	⬆️
... and 11 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 c7c2082...a420d3d. Read the comment docs.

ischoegl · 2021-12-06T02:43:08Z

@speth ... these are some changes that are a preamble to #1089. While the ReactionData reorganization is a follow-up to your ideas in #1151, I am also including the split of GasKinetics::updateROP that #1089 introduces (the reorganization makes individual parts reusable). As this reshuffles things a bit, I wanted to post this now as this may create further conflicts with #1099 (@BangShiuh, sorry!).

ischoegl

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

include/cantera/kinetics/Arrhenius.h

include/cantera/kinetics/Falloff.h

ischoegl · 2021-12-12T15:44:00Z

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

Now that ReactionData has become become a lot 'smarter' with Conditional rate updates for MultiRate reactions #1151, I created an inheritance structure to eliminate redundant code.
The evalFromStruct approach makes the updateFromStruct unnecessary, as after Conditional rate updates for MultiRate reactions #1151 they are only called in combination, so the former can take care of everything
I ended up replacing the 'buffering' approach used for BlowersMaselRate and FalloffRate by a more consistent update approach that adds these values to shared data used by the ReactionRate._evaluator mechanism introduced in Wrap up GasKinetics #1101
I removed some of the service functions that I don't think will be needed, but kept some basic access to test the reaction rate evaluators. While some of your (highly useful!) has_ddT based templates are currently gone, they will come back with Sparse Jacobians for GasKinetics #1089
I also took care of the one remaining papercut where GasKinetics::getThirdBodyConcentrations may access data that are not initialized.

ischoegl · 2021-12-12T17:14:25Z

There was one additional simplification that I left on the table (taken care of now), which also made Python's ReactionRate.__call__ API much cleaner. The PR should be good now.

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.

…alues

ischoegl · 2021-12-13T14:05:15Z

@BangShiuh ... rebased and updated docstrings for the update mechanism (clarifying what is used when). Hope this helps with #1099.

speth

This looks great, @ischoegl. I just had a few very minor comments to be considered.

include/cantera/kinetics/ReactionData.h

include/cantera/kinetics/GasKinetics.h

ischoegl · 2021-12-14T02:58:58Z

@speth ... thanks for the prompt review!

speth

Looks good to me. Thank you, @ischoegl!

ischoegl force-pushed the update-reaction-data branch 2 times, most recently from c2fe0b6 to 26d62ac Compare December 6, 2021 02:03

ischoegl requested a review from a team December 6, 2021 02:36

ischoegl force-pushed the update-reaction-data branch from 26d62ac to ac0c875 Compare December 6, 2021 14:10

ischoegl marked this pull request as draft December 8, 2021 19:50

ischoegl marked this pull request as ready for review December 11, 2021 22:16

ischoegl commented Dec 11, 2021

View reviewed changes

include/cantera/kinetics/Arrhenius.h Outdated Show resolved Hide resolved

include/cantera/kinetics/Falloff.h Outdated Show resolved Hide resolved

ischoegl force-pushed the update-reaction-data branch 5 times, most recently from 24dd022 to dfabbc6 Compare December 12, 2021 15:13

ischoegl changed the title ~~Update ReactionData~~ Update ReactionData and consolidate update strategies Dec 12, 2021

ischoegl force-pushed the update-reaction-data branch from dfabbc6 to 9f74f1d Compare December 12, 2021 15:31

ischoegl requested review from speth and removed request for a team and speth December 12, 2021 15:58

ischoegl marked this pull request as draft December 12, 2021 16:21

ischoegl force-pushed the update-reaction-data branch 2 times, most recently from 50466c5 to f8b4f0a Compare December 12, 2021 17:01

ischoegl marked this pull request as ready for review December 12, 2021 17:13

ischoegl requested a review from speth December 12, 2021 17:14

ischoegl mentioned this pull request Dec 13, 2021

electron-temperature reaction #1099

Merged

5 tasks

ischoegl added 3 commits December 13, 2021 08:03

[Kinetics] Update ReactionRate derivative calculators to use scaled v…

52f98da

…alues

[Kinetics] Create base class for ReactionData

80a2444

ischoegl added 16 commits December 13, 2021 08:03

[Kinetics] Make pressure accessible for all ReactionData structs

92da546

[Kinetics] Remove updateFromStruct from BlowersMaselRate and FalloffRate

fdfc40e

[Kinetics] Remove updateFromStruct from PlogRate and ChebyshevRate3

795073e

[Kinetics] Remove calls for individual rate updates from MultiBulkRate

57fc524

[Kinetics] Simplify MultiBulkRate::update

37d1013

[Kinetics] Implement third parameter to reaction rate evaluators

c3ac3d1

[Kinetics] Remove buffered third-body concentration from Falloff

51d5998

[Kinetics] Remove buffered deltaH from BlowersMaselRate

89abeef

[Cython] Fix BlowersMaselRate inheritance in _cantera.pxd

5676b8e

[Kinetics] Update reaction index when modifying reactions

656fae0

[Kinetics] Remove unnecessary checks

93c0938

[Samples] Update blowers_masel.py example

015fb21

[Kinetics] Ensure getThirdBodyConcentrations is up-to-date

5f41855

[Kinetics] Remove unnecessary service functions

03a1cf1

[Kinetics] Remove unnecessary arguments from ReactionData::update

d019c52

[UnitTests] Update test suite

107070b

ischoegl force-pushed the update-reaction-data branch from f8b4f0a to 1443057 Compare December 13, 2021 14:03

[Kinetics] Improve docstrings for ReactionData update mechanism

4144bff

ischoegl force-pushed the update-reaction-data branch from 1443057 to 4144bff Compare December 13, 2021 14:07

speth requested changes Dec 14, 2021

View reviewed changes

ischoegl force-pushed the update-reaction-data branch from 6edea53 to 285ce29 Compare December 14, 2021 02:58

ischoegl requested a review from speth December 14, 2021 02:59

[Kinetics] Tweak docstrings/formatting and move code from header

a420d3d

ischoegl force-pushed the update-reaction-data branch from 285ce29 to a420d3d Compare December 14, 2021 14:57

speth approved these changes Dec 14, 2021

View reviewed changes

speth merged commit 51a92e3 into Cantera:main Dec 14, 2021

ischoegl deleted the update-reaction-data branch December 14, 2021 17:14

ischoegl mentioned this pull request Dec 29, 2021

Bump version to 2.6.0a4 #1163

Merged

5 tasks

Uh oh!

Conversation

ischoegl commented Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ischoegl commented Dec 6, 2021

Uh oh!

ischoegl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ischoegl commented Dec 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ischoegl commented Dec 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ischoegl commented Dec 13, 2021

Uh oh!

speth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ischoegl commented Dec 14, 2021

Uh oh!

speth left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ischoegl commented Dec 6, 2021 •

edited

Loading

codecov bot commented Dec 6, 2021 •

edited

Loading

ischoegl left a comment •

edited

Loading

ischoegl commented Dec 12, 2021 •

edited

Loading

ischoegl commented Dec 12, 2021 •

edited

Loading