-
-
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
add Tsang closed-form falloff #1090
Conversation
@mefuller ... thanks for submitting! While I am in the midst of refactoring some of Cantera's reaction rate calculation routines (see Cantera/enhancements#87 - adding new classes should become much easier at that point), it should be possible to merge this before I start rewriting all of the Falloff classes. I'll leave this decision to @speth (at the moment, #1088 and #1089 are higher on my own priority list). |
Codecov Report
@@ Coverage Diff @@
## main #1090 +/- ##
==========================================
+ Coverage 73.45% 73.46% +0.01%
==========================================
Files 364 364
Lines 47611 47667 +56
==========================================
+ Hits 34972 35018 +46
- Misses 12639 12649 +10
Continue to review full report at Codecov.
|
Hi @mefuller thanks for submitting this and the related Enhancements issue! A couple of notes from me too as you're working on it, which will hopefully make your life easier:
Thanks! |
@bryanwweber … thank you for the comments. From my perspective, getting unit tests and import right is the most important aspect. If those are in place, porting to the new framework will be straightforward. @mefuller … thanks again for the contribution. As mentioned, I am planning to port Falloff reactions soon, but as this is already implemented, it may be easiest if this is adopted before the transition. Looking forward to your feedback! |
@ischoegl @bryanwweber if either of you has a chance, would you mind taking a look at test Reaction.FalloffFromYaml3 in Once I get the test to pass, I'll clean up the unnecessary changes and squash commits before finalizing the PR. |
Hi @mefuller ... the one remaining error appears to be due to Cantera's internal units checkers. Internally, everything is handled in kg-m-kmol-s, but you use For your new reaction, you need to make sure that the output dimensions check out (they are a function of reaction order) - rate of progress should have the correct dimensions in terms of |
I undid changes for converting from cti and ctml that @bryanwweber indicated were not required and removed the ck2yaml converter changes as the Tsang format has never been officially supported in CHEMKIN, just hacked in by some researchers. |
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.
This looks pretty good to me - added code builds on existing templates for other Falloff
reactions. All of the comments I left should be easy to address.
Beyond, files ck2cti
, ck2yaml
and cti2yaml
don't have any changes and thus should not show up in the diff. Finally, feel free to add yourself to the AUTHORS
file.
Made changes and squashed commits; |
@mefuller The change in the converter files is that they've been made executable by your changes. You can fix it by (for each of the files):
and commit the resulting changes with |
@ischoegl @bryanwweber hopefully everything is now done |
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.
@mefuller ... thank you! - it indeed looks mostly good to me, although there are some minor quibbles. Cantera recommends limiting line length to 88 characters, which some of the docstrings exceed. (yaml input file, but also in Falloff.h
for the docstring of class Tsang
)
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.
@mefuller .. thank you for taking care of the requests. Unless @bryanwweber has any concerns, I will merge later today.
Changes proposed in this pull request
Add Wing Tsang's falloff rate expression (explicit value of F_cent provided with one or two parameters for calculation of F via Troe falloff)
Tsang
falloff whereF_cent = A + B*T
F
is calculated fromF_cent
according to TroeIf applicable, fill in the issue number this pull request is fixing
Closes #
If applicable, provide an example illustrating new features this pull request is introducing
Checklist
scons build
&scons test
) and unit tests address code coverage