Skip to content

Reverse reaction (super-elastic) electron collision reaction #1854

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

Merged
merged 10 commits into from
May 18, 2025

Conversation

BangShiuh
Copy link
Contributor

@BangShiuh BangShiuh commented Feb 27, 2025

Changes proposed in this pull request

The reverse reaction rate of electron collision reaction is added. The reverse rate is calculated super-elastic formula. In the test, a reversible elastic reaction is used for testing purpose only. The reverse rate is the same as the forward rate if the threshold energy is zero.

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

Cantera/enhancements#192

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

Copy link

codecov bot commented Feb 27, 2025

Codecov Report

Attention: Patch coverage is 83.07692% with 11 lines in your changes missing coverage. Please review.

Project coverage is 74.13%. Comparing base (932f899) to head (6fdf863).
Report is 23 commits behind head on main.

Files with missing lines Patch % Lines
src/kinetics/ElectronCollisionPlasmaRate.cpp 73.07% 3 Missing and 4 partials ⚠️
src/kinetics/BulkKinetics.cpp 89.47% 0 Missing and 2 partials ⚠️
src/kinetics/InterfaceKinetics.cpp 84.61% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1854   +/-   ##
=======================================
  Coverage   74.13%   74.13%           
=======================================
  Files         443      443           
  Lines       55400    55423   +23     
  Branches     9107     9114    +7     
=======================================
+ Hits        41069    41089   +20     
  Misses      11241    11241           
- Partials     3090     3093    +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@BangShiuh BangShiuh force-pushed the reverse-rate branch 2 times, most recently from 1f768e5 to 4f26456 Compare March 9, 2025 19:36
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @BangShiuh, I think this looks good, and seems like a reasonable time to merge in my implementation of ReactionRate.modifyRateConstants. While this had originally been done with electrochemical rate parameterizations in mind, I'm glad to see that it's general enough to be useful here as well. I have just a couple of minor cleanup suggestions.

@BangShiuh BangShiuh force-pushed the reverse-rate branch 2 times, most recently from 4f720c5 to 09c23e0 Compare May 18, 2025 01:07
@BangShiuh
Copy link
Contributor Author

@speth Thank you for the code review. The issues have been fixed.

@speth
Copy link
Member

speth commented May 18, 2025

@ischoegl - Since this incorporates an extension of the ReactionRate / MultiRate interface that I wrote a while back, do you want to look this over and approve? This was written with the intention of it being used to generalize electrochemical rate handling (which has previously been discussed in Cantera/enhancements#38 and Cantera/enhancements#44, though not this specific implementation).

Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@speth ... sure - I looked over the three commits and only found one virtual that is no longer needed.

@@ -1203,7 +1203,7 @@ class Kinetics
* @param i reaction index
*/
virtual bool isReversible(size_t i) {
throw NotImplementedError("Kinetics::isReversible");
return std::find(m_revindex.begin(), m_revindex.end(), i) < m_revindex.end();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this won't need to be virtual any longer, correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and it should be const as well.

@ischoegl ischoegl merged commit f6fdbd6 into Cantera:main May 18, 2025
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants