Merge GasKinetics into BulkKinetics and simplify rate updates #1483
Conversation
speth
force-pushed
the
deprecate-gaskinetics
branch
from
68c463f
to
e7cff52
Compare
speth
force-pushed
the
deprecate-gaskinetics
branch
from
e7cff52
to
903004f
Compare
Codecov Report
@@ Coverage Diff @@
## main #1483 +/- ##
==========================================
- Coverage 70.14% 70.12% -0.02%
==========================================
Files 377 376 -1
Lines 58002 57977 -25
Branches 19422 19429 +7
==========================================
- Hits 40687 40658 -29
- Misses 14754 14755 +1
- Partials 2561 2564 +3
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -82,10 +82,12 @@ unique_ptr<Kinetics> newKinetics(const std::vector<ThermoPhase*>& phases, | |||
| * the reactions occur. Searches the Cantera data for this file. | |||
| * @param phase_name The name of the reacting phase in the input file (that is, the | |||
| * name of the first phase in the `phases` vector) | |||
| * @deprecated The 'phase_name' argument is deprecated and will be removed after | |||
| * Cantera 3.1. | |||
There was a problem hiding this comment.
Could you add a comment about the new behavior?
src/kinetics/KineticsFactory.cpp
Outdated
| if (phase_name != "" && phase_name != phases.at(0)->name()) { | ||
| warn_deprecated("newKinetics", "The reacting phase must be the first phase in " | ||
| "the 'phases' vector. Remove the 'phase_name' argument to use the first " | ||
| " phase by default. This warning will be an error after Cantera 3.0. "); |
There was a problem hiding this comment.
Isn’t the parameter removed after Cantera 3.0?
There was a problem hiding this comment.
The phase_name parameter is gone, but having the reacting phase not in first position will raise an exception from Kinetics::addThermo.
There was a problem hiding this comment.
I guess I was puzzled by "this warning will be an error", as the clause would have to go somewhere else?
There was a problem hiding this comment.
OK, I think I was making this part too complicated -- the phase_name parameter can be deprecated immediately. Specifying the first phase's name to newKinetics can issue a warning, and having the reacting phase in a different order will issue a warning from Kinetics::addThermo.
speth
force-pushed
the
deprecate-gaskinetics
branch
3 times, most recently
from
a408283
to
d388e95
Compare
Standardize on the behavior implemented by the newSolution and newInterface constructors.
speth
force-pushed
the
deprecate-gaskinetics
branch
from
d388e95
to
50a5211
Compare
Changes proposed in this pull request
With all specializations for different reaction rate parameterizations now handled by
ReactionRateclasses, there is very little left in theGasKineticsclass that doesn't apply to kinetics in any bulk phase. From this point, I think there should be no need to specializeBulkKineticsfor the purpose of representing different thermodynamic or kinetic models -- these should be handled by the specializations ofThermoPhaseandReactionRate.GasKineticstoBulkKineticsand deprecate classGasKineticsupdateROPInterfaceKineticsobjects with the reacting phase not as the first phaseChecklist
scons build&scons test) and unit tests address code coverage