Skip to content
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

Proof-of-Concept: Eliminate reaction specializations #1219

Closed

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Mar 15, 2022

Changes proposed in this pull request

Remove remaining specializations per Cantera/enhancements#142

  • Create ThreeBodyArrheniusRate specialization for three-body-Arrhenius
  • Eliminate ThreeBodyReaction in C++ and Python
  • 🚧

Once fully implemented, this PR will allow for a deprecation of ReactionFactory.

If applicable, fill in the issue number this pull request is fixing

Closes Cantera/enhancements#142, closes Cantera/enhancements#133, interferes with #1143

If applicable, provide an example illustrating new features this pull request is introducing

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

Thoughts

Due to the introduction of BulkRate.h, this PR has some minor conflicts with #1217

@codecov
Copy link

codecov bot commented Mar 15, 2022

Codecov Report

Merging #1219 (c93ea76) into main (719275e) will decrease coverage by 0.17%.
The diff coverage is 32.38%.

❗ Current head c93ea76 differs from pull request most recent head 8ecc5da. Consider uploading reports for the commit 8ecc5da to get more accurate results

@@            Coverage Diff             @@
##             main    #1219      +/-   ##
==========================================
- Coverage   65.43%   65.26%   -0.18%     
==========================================
  Files         320      322       +2     
  Lines       46249    46368     +119     
  Branches    19657    19724      +67     
==========================================
- Hits        30265    30264       -1     
- Misses      13454    13568     +114     
- Partials     2530     2536       +6     
Impacted Files Coverage Δ
include/cantera/kinetics/Reaction.h 100.00% <ø> (ø)
include/cantera/kinetics/ReactionData.h 87.27% <0.00%> (-1.62%) ⬇️
src/kinetics/BulkRate.cpp 0.00% <0.00%> (ø)
src/kinetics/BulkKinetics.cpp 84.84% <14.28%> (-3.96%) ⬇️
include/cantera/kinetics/BulkRate.h 14.63% <14.63%> (ø)
src/kinetics/Reaction.cpp 77.85% <30.13%> (-3.39%) ⬇️
include/cantera/kinetics/Arrhenius.h 81.91% <47.82%> (-0.56%) ⬇️
src/kinetics/ReactionRateFactory.cpp 91.93% <50.00%> (-2.90%) ⬇️
src/kinetics/Custom.cpp 77.77% <66.66%> (-5.56%) ⬇️
include/cantera/kinetics/Custom.h 50.00% <100.00%> (-35.72%) ⬇️
... and 8 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@ischoegl ischoegl force-pushed the eliminate-reaction-specializations branch from db429f0 to f787cae Compare March 15, 2022 18:29
@ischoegl ischoegl force-pushed the eliminate-reaction-specializations branch 2 times, most recently from c93ea76 to 1a47f0e Compare March 16, 2022 16:42
@ischoegl ischoegl force-pushed the eliminate-reaction-specializations branch from 1a47f0e to 1270d66 Compare March 16, 2022 22:26
@ischoegl ischoegl force-pushed the eliminate-reaction-specializations branch from 1270d66 to 8ecc5da Compare March 17, 2022 04:44
@ischoegl
Copy link
Member Author

@speth ... I was (mostly) successful replacing ThreeBodyReaction, but the current approach does have an estimated 15% speed penalty (note that at the moment, effective three-body concentrations are calculated twice, but disabling updateFromStruct in the ThreeBodyRate<> template is not the culprit). The existing approach of handling third body colliders outside of ReactionRate is simply faster, which means that the separation of BulkRate<> and ThreeBodyRate<> templates that this PR tested is not worth pursuing.

While I will not have the bandwidth at the moment to go back and work out a hybrid approach where an updated ThirdBody object gains the ability to parse (which I believe is what is necessary), much of the machinery to catch three-body reactions is worked out. I believe the feature is overall worth implementing, as it will eliminate ReactionFactory and allow for some significant simplifications along the way.

@ischoegl ischoegl changed the title Eliminate reaction specializations Proof-of-Concept: Eliminate reaction specializations Mar 17, 2022
@ischoegl ischoegl closed this Mar 24, 2022
@ischoegl ischoegl deleted the eliminate-reaction-specializations branch July 31, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant