-
-
Notifications
You must be signed in to change notification settings - Fork 349
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
Preamble to InterfaceKinetics refactoring #1184
Conversation
3fe51f0
to
04629b0
Compare
Codecov Report
@@ Coverage Diff @@
## main #1184 +/- ##
==========================================
+ Coverage 65.39% 65.44% +0.05%
==========================================
Files 318 318
Lines 46109 46106 -3
Branches 19601 19604 +3
==========================================
+ Hits 30153 30175 +22
+ Misses 13452 13435 -17
+ Partials 2504 2496 -8
Continue to review full report at Codecov.
|
847a3a5
to
a1c4b5c
Compare
906afe7
to
f5008ce
Compare
@speth ... this should take care of a lot of accumulated lint, and clearly mark legacy objects wherever used. Efficient handling of the There should be only minimal overlap with #1183 (if any). I added one instance of |
Co-authored-by: Ray Speth <[email protected]>
f5008ce
to
74650d2
Compare
ChebyshevRate3 was introduced to avoid a conflict with the pre-existing class 'ChebyshevRate'. Since its introduction, the conflict was removed. This commit restores the original name to this class.
Co-authored-by: Ray Speth <[email protected]>
74650d2
to
7a6f1bd
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 splitting this out, since a lot of this is just reorganization and name changes. I think it will make reviewing #1181 a fair bit easier.
I like the addition of setContext
-- it makes a lot of sense to have a function that can be called to set things like species indices that can be resolved at the point when a reaction is added to a Kinetics object, but not before.
This comment was marked as outdated.
This comment was marked as outdated.
63e2f88
to
440a563
Compare
2709a41
to
5fa23b7
Compare
@speth ... I addressed all comments. GH actions appears down for the time being; I'll try to re-trigger later on. Regarding the renaming of |
GH actions are back up, and everything ran through … |
7dbc17d
to
3c0c53d
Compare
- Add todo item for deprecation of Arrhenius2 constructor - Add missing override declarations in BlowersMaselRate
3c0c53d
to
ce94ac1
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. This looks good to me.
Changes proposed in this pull request
This PR seeks to address several issues that came up while working on #1181.
NO_LEGACY_REACTIONS_26
is setactivationEnergy
in the context ofBlowersMasel
... useeffectiveActivationEnergy
for version that includes dependency on thermodynamic state (same will be introduced forReactionRate
classes that handle interface reactions).Arrhenius
... clarify asArrhenius2
in legacy framework and make it equivalent withArrheniusBase
in new framework (ArrheniusBase
should be renamed toArrhenius
after Cantera 2.6). Note: Defining a reducedArrhenius(Base)
class will also allow for additional templated classes - see e.g. Rethink handling of 3rd body colliders enhancements#133.ChebyshevRate(3)
deltaH
withinBlowersMasel
without requiring associatedKinetics
objectIf applicable, fill in the issue number this pull request is fixing
If applicable, provide an example illustrating new features this pull request is introducing
This PR will allow to disable legacy reactions (actually
typedef
s) fromSCons
asChecklist
scons build
&scons test
) and unit tests address code coverage