-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
Refactor interface kinetics #1181
Conversation
a4184d4
to
e6db0ac
Compare
e6db0ac
to
d068924
Compare
Codecov Report
@@ Coverage Diff @@
## main #1181 +/- ##
==========================================
+ Coverage 65.38% 65.42% +0.04%
==========================================
Files 318 320 +2
Lines 46095 46247 +152
Branches 19604 19655 +51
==========================================
+ Hits 30139 30259 +120
- Misses 13442 13460 +18
- Partials 2514 2528 +14
Continue to review full report at Codecov.
|
ba42aa7
to
1b190f7
Compare
45edcd0
to
d211185
Compare
652cb5a
to
a4d5515
Compare
Raise deprecation warnings for legacy properties of deprecated Reaction specializations.
* Use prefix `sticking-` instead of suffix `-stick` * Use prefix `interface-` instead of suffix `-interface` * Rename associated objects, e.g. ArrheniusStickRate -> StickingArrheniusRate * Also, additional minor clarifications
57db852
to
aef42c3
Compare
@speth … thank you for the comments! I addressed all except …
The behavior is definitely not what I expected either. I can think of something (updating when adding reactions), but my first tests were not successful. I need to make sure that the state of the phase is set before reactions are added … which may or may not be already the case (fwiw, I didn’t get the expected results). I‘ll try again later today.
This is correct, and using serialization here was exactly my rationale for eliminating a couple of methods. |
>>> iface = ct.Interface('ptcombust.yaml', 'Pt_surf')
>>> R = iface.reaction(1)
>>> print(R.rate.activation_energy)
67400000.0 # this is the base value with no coverage dependence
>>> R.rate(500, iface.coverages)
>>> print(R.rate.activation_energy)
64400000.0 # now this includes the coverage dependence
@speth ... you are probably aware of this, but the behavior is due to the fact that A resolution would require something akin to #1051, but I'm not sure that I want to include this in |
ReactionData::update (and updateFromStruct of corresponding Reactions) are currently run during first call of updateROP.
eee4f58
to
547c737
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 for the updates, @ischoegl. The update to the automatic sticking species detection looks good to me.
I agree that the synchronization issues is better handled outside of this PR, but is something that I think we need to deal with, so thanks for creating a new issue for that.
Changes proposed in this pull request
Refactor
InterfaceKinetics
objects to reflect recent changes inGasKinetics
.SurfaceArrhenius
byInterfaceRate<>
andStickRate<>
class templatesInterfaceKinetics
to use new frameworkElectrochemicalReaction
remains for the time being (the PR is getting long, and some issues pertaining to Improve handling of electrochemical reactions enhancements#38 remain to be discussed)Checklist:
Reaction
specializations in PythonIf applicable, fill in the issue number this pull request is fixing
Closes Cantera/enhancements#87, closes Cantera/enhancements#8, closes #1068, closes #1210
If applicable, provide an example illustrating new features this pull request is introducing
Checklist
scons build
&scons test
) and unit tests address code coverage