-
-
Notifications
You must be signed in to change notification settings - Fork 374
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
Conversation
Codecov ReportAttention: Patch coverage is
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. 🚀 New features to boost your workflow:
|
1f768e5
to
4f26456
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.
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.
4f720c5
to
09c23e0
Compare
@speth Thank you for the code review. The issues have been fixed. |
@ischoegl - Since this incorporates an extension of the |
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 ... 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(); |
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.
I believe this won't need to be virtual any longer, correct?
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.
Correct, and it should be const
as well.
This should enable a clean implementation of the Butler-Volmer and Marcus forms of electrochemical reactions.
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
scons build
&scons test
) and unit tests address code coverage