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

Modify third body efficiencies using modify_reaction #1143

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lavdwall
Copy link
Contributor

Changes proposed in this pull request

Until now, when modifying a reaction using the modifyReaction function in C++ (or modify_reaction from Python), only the rates are modified, not the third-body efficiencies. Although the composition map that stores the efficiencies can be modified from Python (using get/set function efficiencies), these modifications are not taken into account when consequently calculating the reaction rates.

In this pull request, the functionality of modifyReaction is extended to also include modifications to third-body efficiencies. This is possible with only a few additions to GasKinetics.cpp and ThirdBodyCalc.h

For testing, I modified the add_falloff_reaction test in kineticsFromScratch.cpp: I added a reaction with wrong third-body-efficiencies first, verified that this gives wrong results, then used modifyReaction to correct the third-body efficiencies and verify the result is as expected.

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

@lavdwall lavdwall changed the title Third body efficiencies Modify third body efficiencies using modify_reaction Nov 24, 2021
@codecov
Copy link

codecov bot commented Nov 24, 2021

Codecov Report

Merging #1143 (4ec6d7e) into main (0e2a33d) will increase coverage by 0.00%.
The diff coverage is 84.21%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1143   +/-   ##
=======================================
  Coverage   73.50%   73.51%           
=======================================
  Files         365      365           
  Lines       48247    48284   +37     
=======================================
+ Hits        35466    35497   +31     
- Misses      12781    12787    +6     
Impacted Files Coverage Δ
src/kinetics/GasKinetics.cpp 90.23% <50.00%> (-2.38%) ⬇️
include/cantera/kinetics/ThirdBodyCalc.h 100.00% <100.00%> (ø)
test/kinetics/kineticsFromScratch.cpp 100.00% <100.00%> (ø)

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 0e2a33d...4ec6d7e. Read the comment docs.

@ischoegl
Copy link
Member

ischoegl commented Dec 7, 2021

Hi @lavdwall ... thanks for the pull request! While I believe the objective of modifying the collision efficiencies is well conceived, there has been some ongoing work on the code base, which unfortunately affects your work. Cantera 2.6 will introduce significant changes in the way reactions and associated rates are handled (see Cantera/enhancements#87, a substantial portion was recently merged with #1101). Specifically, your PR targets ThreeBodyReaction2, together with GasKinetics::modifyThreeBodyReaction and member variables GasKinetics::m_3b_concm and GasKinetics::m_falloff_concm, all of which are de-facto deprecated and replaced by new handlers. At this point, anything loaded from YAML input bypasses these methods.

The way things are handled going forward involves a shared pointer Reaction::thirdBody() which is only initialized for ThreeBodyReaction3 and FalloffReaction3. Similarly, testing should involve kineticsFromScratch3.cpp. As an aside, the internal extension 3 is used for the new framework; everything marked with 2 will be deprecated in 2.6, and removed for the major 3.0 release.

@lavdwall
Copy link
Contributor Author

lavdwall commented Dec 7, 2021

Hm, I see.. Thanks for the clarification! I tested it two weeks ago on a mechanism loaded from YAML and back then everything still worked correctly, but that was before #1101 :)
I will look at the details of the new implementation. Is it worth it if I look at implementing this feature in the new framework, or is the possibility to modify third-body efficiencies there by default? If it is not worth it, I will just continue with my old version of code for now.

@ischoegl
Copy link
Member

ischoegl commented Dec 7, 2021

Yes, the changes hit main very recently (at least for Falloff).

Is it worth it if I look at implementing this feature in the new framework, or is the possibility to modify third-body efficiencies there by default?

Good question: the setters all appear to be there, but that is somewhat misleading: similar to the handling of reaction rates, things won't get updated for GasKinetics internally once a reaction is added (which is why modify_reaction is important, which however lacks the ability to change efficiencies). So what you have is still relevant; note that there now is a ThirdBodyCalc3 ...

@ischoegl
Copy link
Member

ischoegl commented Feb 9, 2022

Fwiw, Cantera/enhancements#133 may implicitly address this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants