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

Remove legacy kinetics #1292

Merged
merged 25 commits into from
May 28, 2022
Merged

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented May 25, 2022

Changes proposed in this pull request

  • Remove infrastructure used by legacy Kinetics evaluators
  • Legacy code was used by objects imported from CTI/XML input (removed in Remove deprecated CTI & XML formats #1291); new infrastructure is used by default when importing from YAML since Cantera 2.6.0

If applicable, fill in the issue number this pull request is fixing

Closes Cantera/enhancements#149

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 May 25, 2022

Codecov Report

Merging #1292 (9872177) into main (3947980) will decrease coverage by 0.16%.
The diff coverage is 82.08%.

❗ Current head 9872177 differs from pull request most recent head e6d59a3. Consider uploading reports for the commit e6d59a3 to get more accurate results

@@            Coverage Diff             @@
##             main    #1292      +/-   ##
==========================================
- Coverage   67.42%   67.26%   -0.17%     
==========================================
  Files         320      314       -6     
  Lines       42890    41887    -1003     
  Branches    17353    16852     -501     
==========================================
- Hits        28920    28174     -746     
+ Misses      11670    11483     -187     
+ Partials     2300     2230      -70     
Impacted Files Coverage Δ
include/cantera/kinetics/Arrhenius.h 90.00% <ø> (ø)
include/cantera/kinetics/GasKinetics.h 100.00% <ø> (ø)
include/cantera/kinetics/InterfaceKinetics.h 85.71% <ø> (ø)
include/cantera/kinetics/Kinetics.h 35.65% <ø> (ø)
include/cantera/kinetics/Reaction.h 94.44% <ø> (-5.56%) ⬇️
include/cantera/kinetics/ThirdBodyCalc.h 98.38% <ø> (-0.52%) ⬇️
src/base/application.cpp 50.00% <ø> (-1.27%) ⬇️
src/base/application.h 70.45% <ø> (ø)
src/clib/ct.cpp 7.68% <0.00%> (ø)
src/fortran/fct.cpp 19.96% <0.00%> (-0.19%) ⬇️
... and 27 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@ischoegl ischoegl force-pushed the remove-legacy-kinetics branch 7 times, most recently from 00d2afa to 8ea3516 Compare May 26, 2022 04:14
@ischoegl ischoegl marked this pull request as ready for review May 26, 2022 04:30
@ischoegl ischoegl force-pushed the remove-legacy-kinetics branch 3 times, most recently from 056c4eb to 7d26a94 Compare May 26, 2022 05:32
@ischoegl ischoegl requested a review from a team May 26, 2022 11:23
Copy link
Member

@bryanwweber bryanwweber 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 this @ischoegl! I love the ratio of +/- lines in these last few PRs 😃 I've gotta work on that myself! Just a few minor comments about formatting here. I'll let @speth review the more substantive changes to the C++ code, since I'm not all that familiar with the appropriate data types, etc.

SConstruct Outdated Show resolved Hide resolved
include/cantera/kinetics/ChebyshevRate.h Show resolved Hide resolved
include/cantera/kinetics/ChebyshevRate.h Show resolved Hide resolved
include/cantera/kinetics/Kinetics.h Outdated Show resolved Hide resolved
include/cantera/kinetics/PlogRate.h Outdated Show resolved Hide resolved
src/kinetics/Kinetics.cpp Outdated Show resolved Hide resolved
test/kinetics/kineticsFromScratch.cpp Show resolved Hide resolved
test/kinetics/kineticsFromScratch.cpp Show resolved Hide resolved
test/kinetics/kineticsFromScratch.cpp Show resolved Hide resolved
test/kinetics/kineticsFromScratch.cpp Outdated Show resolved Hide resolved
@bryanwweber
Copy link
Member

One other question I wanted to ask, is it necessary to keep the option to change the default status of the legacy rate constants at compile time? I think it makes sense to keep the ability to change that on the fly, but I don't think it's super helpful to keep the compile-time option. I'm worried that it will lead some research group to install a modified copy on their server, and we'll get questions on the UG that we can't figure out because the end-user doesn't know that option was changed.

@ischoegl
Copy link
Member Author

ischoegl commented May 26, 2022

@bryanwweber ... thanks for the prompt review! And I was able to shave a couple more lines ... removing this was definitely easier than coding up what replaces the legacy code 😂

One other question I wanted to ask, is it necessary to keep the option to change the default status of the legacy rate constants at compile time? I think it makes sense to keep the ability to change that on the fly, but I don't think it's super helpful to keep the compile-time option. I'm worried that it will lead some research group to install a modified copy on their server, and we'll get questions on the UG that we can't figure out because the end-user doesn't know that option was changed.

Fair point - I removed this in the last commit.

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, @ischoegl. I likewise enjoy the net decrease in lines of code here.

Besides a couple of specific comments below, I think there are several cases where we need to keep the Whatever3 names as typedefs or passthroughs, at least for the next version. This is important for enabling code that is built to work with Cantera 2.6 without using any deprecated features to still work with the development version of Cantera 3.0. For example:

  • ThirdBodyCalc3
  • Reaction::calculateRateCoeffUnits3
  • ThreeBodyReaction3
  • FalloffReaction3
  • Kinetics.reactant_stoich_coeffs3 and product_stoich_coeffs3

include/cantera/kinetics/Arrhenius.h Outdated Show resolved Hide resolved
include/cantera/kinetics/Kinetics.h Outdated Show resolved Hide resolved
src/fortran/cantera_kinetics.f90 Outdated Show resolved Hide resolved
src/clib/ct.cpp Outdated Show resolved Hide resolved
src/kinetics/Kinetics.cpp Show resolved Hide resolved
* Change behavior of Kinetics::reactionType
* Deprecate Kinetics::reactionTypeStr
* Change f90/clib getters to getReactionType pattern

update getReactionType
@ischoegl ischoegl force-pushed the remove-legacy-kinetics branch 2 times, most recently from 85b419b to 2a65d37 Compare May 28, 2022 13:59
@ischoegl
Copy link
Member Author

@speth ... thanks for the review! I added all suggestions (except for Kinetics.reactant_stoich_coeffs3 and product_stoich_coeffs3 pass-throughs, which were already in place).

I think this is ready to go ...

Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more small doc change for consistency 🙂

src/base/application.h Outdated Show resolved Hide resolved
test/kinetics/kineticsFromScratch.cpp Show resolved Hide resolved
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, @ischoegl. Just one minor suggestion for the docs.

include/cantera/kinetics/Kinetics.h Outdated Show resolved Hide resolved
@ischoegl
Copy link
Member Author

@speth / @bryanwweber … let me know in case there’s anything else …

Copy link
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ischoegl !

@bryanwweber bryanwweber merged commit c040b12 into Cantera:main May 28, 2022
@ischoegl ischoegl deleted the remove-legacy-kinetics branch May 28, 2022 21:28
@ischoegl ischoegl mentioned this pull request Sep 10, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Remove legacy Kinetics infrastructure for Cantera 3.0
3 participants