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

Deprecate redundant kinetics methods in Python API #1202

Merged

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Feb 20, 2022

Changes proposed in this pull request

  • Deprecate redundant / unpythonic methods that mimic the C-API (all alternative methods already exist):
    • Kinetics.reaction_type(i) -> Kinetics.reaction(i).reaction_type
    • Kinetics.is_reversible(i) -> Kinetics.reaction(i).reversible
    • Kinetics.reaction_equation(i) -> Kinetics.reaction(i).equation
    • Kinetics.reactants(i) -> Kinetics.reaction(i).reactant_string
    • Kinetics.products(i) -> Kinetics.reaction(i).product_string
  • Improve rendering of Reaction.__repr__, where essential information was lost in Eliminate unnecessary specialized Reaction types #1183
  • Fix indentation and blank lines for Sphinx docstring deprecation information

If applicable, provide an example illustrating new features this pull request is introducing

After #1183, several reactions no longer show specialized information (which is held by the ReactionRate), where specifically no identifiable information is shown for Chebyshev and pressure-dependent-Arrhenius (Plog) (as well as the new Blowers-Masel and two-temperature-plasma types), e.g.

in [1]: import cantera as ct
   ...: gas = ct.Solution("test/data/kineticsfromscratch.yaml")
   ...: gas.reactions()
Out[1]: 
[<Reaction: H2 + O <=> H + OH>,
 <ThreeBodyReaction: 2 O + M <=> O2 + M>,
 <FalloffReaction: 2 OH (+M) <=> H2O2 (+M)>,
 <Reaction: H2 + O2 <=> 2 OH>,
 <Reaction: HO2 <=> O + OH>,
 <ThreeBodyReaction: H + O2 + O2 <=> HO2 + O2>,
 <Reaction: H2 + O <=> H + OH>,
 <FalloffReaction: 2 OH (+M) <=> H2O2 (+M)>,
 <FalloffReaction: H + HO2 (+M) <=> H2 + O2 (+M)>,
 <FalloffReaction: H + HO2 (+M) <=> H2 + O2 (+M)>,
 <ChemicallyActivatedReaction: H2O + OH (+M) <=> H2 + HO2 (+M)>,
 <Reaction: H + O => H + O>,
 <Reaction: O + OH => O + OH>]

This PR reformats Reaction.__repr__ to always include information for the rate specialization.

In [1]: import cantera as ct
   ...: gas = ct.Solution("test/data/kineticsfromscratch.yaml")
   ...: gas.reactions()
Out[1]: 
[H2 + O <=> H + OH    <Reaction(Arrhenius)>,
 2 O + M <=> O2 + M    <ThreeBodyReaction(Arrhenius)>,
 2 OH (+M) <=> H2O2 (+M)    <FalloffReaction(Troe)>,
 H2 + O2 <=> 2 OH    <Reaction(pressure-dependent-Arrhenius)>,
 HO2 <=> O + OH    <Reaction(Chebyshev)>,
 H + O2 + O2 <=> HO2 + O2    <ThreeBodyReaction(Arrhenius)>,
 H2 + O <=> H + OH    <Reaction(Blowers-Masel)>,
 2 OH (+M) <=> H2O2 (+M)    <FalloffReaction(Lindemann)>,
 H + HO2 (+M) <=> H2 + O2 (+M)    <FalloffReaction(SRI)>,
 H + HO2 (+M) <=> H2 + O2 (+M)    <FalloffReaction(Tsang)>,
 H2O + OH (+M) <=> H2 + HO2 (+M)    <ChemicallyActivatedReaction(Lindemann)>,
 H + O => H + O    <Reaction(two-temperature-plasma)>,
 O + OH => O + OH    <Reaction(two-temperature-plasma)>]

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

@ischoegl ischoegl force-pushed the deprecate-redundant-kinetics-methods branch from 1f0098c to 8ce1924 Compare February 20, 2022 15:24
@ischoegl ischoegl changed the title Deprecate redundant kinetics methods Deprecate redundant kinetics methods in Python API Feb 20, 2022
@ischoegl ischoegl force-pushed the deprecate-redundant-kinetics-methods branch from 8ce1924 to ab9c4ab Compare February 20, 2022 16:04
The method (introduced in f0868c7) is not part of a stable release and holds
was intended to facilitate transitional behavior of the Kinetics.reaction_type
method. Instead, the Kinetics.reaction_type method is completely replaced by the
ReactionRate.type property of a specific Reaction.
Deprecated methods follow the C-API style for reaction-specific information
and should be accessed from the associated Reaction object instead.
@codecov
Copy link

codecov bot commented Feb 20, 2022

Codecov Report

Merging #1202 (0889b09) into main (041559d) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1202      +/-   ##
==========================================
- Coverage   65.41%   65.38%   -0.03%     
==========================================
  Files         318      318              
  Lines       46085    46095      +10     
  Branches    19604    19604              
==========================================
- Hits        30145    30139       -6     
- Misses      13426    13442      +16     
  Partials     2514     2514              
Impacted Files Coverage Δ
src/base/application.h 69.76% <0.00%> (-15.95%) ⬇️
src/base/global.cpp 70.58% <0.00%> (-4.26%) ⬇️
include/cantera/base/global.h 81.81% <0.00%> (-2.40%) ⬇️
src/base/application.cpp 63.98% <0.00%> (-1.26%) ⬇️
src/kinetics/Kinetics.cpp 72.47% <0.00%> (-0.46%) ⬇️
include/cantera/cython/wrappers.h 85.15% <0.00%> (-0.33%) ⬇️
src/kinetics/Reaction.cpp 81.07% <0.00%> (+0.17%) ⬆️
src/kinetics/Falloff.cpp 85.39% <0.00%> (+0.74%) ⬆️
include/cantera/base/logger.h 78.57% <0.00%> (+5.84%) ⬆️

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 041559d...0889b09. Read the comment docs.

@ischoegl ischoegl force-pushed the deprecate-redundant-kinetics-methods branch from ab9c4ab to ed1571e Compare February 20, 2022 16:36
@ischoegl ischoegl marked this pull request as ready for review February 20, 2022 16:39
speth
speth previously approved these changes Mar 5, 2022
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.

Looks good to me -- I just had one minor suggestion, but I don't think it affects much either way.

Longer term, I'd like to eliminate these from C++ as well, though that would require updating the C / Matlab / Fortran APIs, which currently have no notion of a Reaction object.

interfaces/cython/cantera/kinetics.pyx Outdated Show resolved Hide resolved
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! A few small comments.

interfaces/cython/cantera/kinetics.pyx Outdated Show resolved Hide resolved
interfaces/cython/cantera/kinetics.pyx Outdated Show resolved Hide resolved
interfaces/cython/cantera/kinetics.pyx Outdated Show resolved Hide resolved
interfaces/cython/cantera/test/test_composite.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/test/test_kinetics.py Outdated Show resolved Hide resolved
@ischoegl ischoegl force-pushed the deprecate-redundant-kinetics-methods branch from b9e3e56 to 406ef98 Compare March 5, 2022 16:54
@ischoegl ischoegl force-pushed the deprecate-redundant-kinetics-methods branch from 406ef98 to 0889b09 Compare March 5, 2022 17:08
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 looks good to me! I'll wait for @speth to approve as well, since he also left a review.

@bryanwweber
Copy link
Member

Ah, I see @speth approved. Feel free to :shipit: once the checks are all green.

@ischoegl ischoegl merged commit 551a45c into Cantera:main Mar 5, 2022
@ischoegl ischoegl deleted the deprecate-redundant-kinetics-methods branch March 5, 2022 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants