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

Refactor interface kinetics #1181

Merged
merged 42 commits into from Mar 12, 2022
Merged

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Jan 20, 2022

Changes proposed in this pull request

Refactor InterfaceKinetics objects to reflect recent changes in GasKinetics.

  • Replace SurfaceArrhenius by InterfaceRate<> and StickRate<> class templates
  • Update InterfaceKinetics to use new framework
  • Exception: ElectrochemicalReaction 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:

  • Update C test suite
  • Issue warnings for deprecated Reaction specializations in Python
  • Use new framework by default when importing from YAML

If 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

  • 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

@codecov
Copy link

codecov bot commented Jan 21, 2022

Codecov Report

Merging #1181 (547c737) into main (551a45c) will increase coverage by 0.04%.
The diff coverage is 85.23%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
include/cantera/kinetics/InterfaceKinetics.h 85.71% <ø> (ø)
include/cantera/kinetics/MultiRateBase.h 100.00% <ø> (ø)
include/cantera/kinetics/RateCoeffMgr.h 85.71% <ø> (-14.29%) ⬇️
src/base/AnyMap.cpp 85.10% <ø> (ø)
src/kinetics/RxnRates.cpp 92.72% <ø> (+1.81%) ⬆️
include/cantera/kinetics/ReactionData.h 88.88% <42.85%> (-4.87%) ⬇️
src/kinetics/InterfaceKinetics.cpp 75.79% <78.43%> (-3.17%) ⬇️
src/kinetics/ReactionData.cpp 85.63% <79.59%> (+0.60%) ⬆️
src/kinetics/ReactionFactory.cpp 89.89% <81.81%> (-0.16%) ⬇️
src/kinetics/Reaction.cpp 81.23% <82.69%> (+0.16%) ⬆️
... and 20 more

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 551a45c...547c737. Read the comment docs.

@ischoegl ischoegl force-pushed the refactor-interface-kinetics branch from 57db852 to aef42c3 Compare March 5, 2022 18:44
@ischoegl
Copy link
Member Author

ischoegl commented Mar 6, 2022

@speth … thank you for the comments! I addressed all except …

The fact that the initial value doesn't correspond to the state of the Interface object is definitely surprising […]. Should we at least be updating the rate when you first access the Reaction object? Is there any useful way to tie this to the Kinetics object? I can think of some complicated ones, but I'm not sure they're worth it.

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.

Also, it seems that there is no longer a way to get the base value of this parameter, although I guess you can always get it by looking at the input_data, which might be fine.

This is correct, and using serialization here was exactly my rationale for eliminating a couple of methods.

@ischoegl
Copy link
Member Author

ischoegl commented Mar 6, 2022

[...] trying this out I noticed that the behavior of ArrheniusInterfaceRate.activation_energy and friends is very non-intuitive:

>>> 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

[...] Is there any useful way to tie this to the Kinetics object? I can think of some complicated ones, but I'm not sure they're worth it.

@speth ... you are probably aware of this, but the behavior is due to the fact that R.rate holds the original ReactionRate object that is created when the Reaction is first instantiated, which is separate from the copy that Kinetics uses to actually evaluate the rate. In my tests, I was partially successful in updating ReactionData right away (the best entry point is likely around the Kinetics::resizeReactions specializations), but at the moment there is no way of probing the content.

A resolution would require something akin to #1051, but I'm not sure that I want to include this in 2.6 this PR. There may be a way to copy the initial state over, but it would be imho more important to keep things synchronized at all times. PS: I opened #1211 as this goes well beyond the changes proposed in this PR.

ReactionData::update (and updateFromStruct of corresponding Reactions) are
currently run during first call of updateROP.
@ischoegl ischoegl force-pushed the refactor-interface-kinetics branch from eee4f58 to 547c737 Compare March 7, 2022 14:55
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 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.

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