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

add integrator parameters to the InterfaceKinetics::advanceCoverages … #610

Conversation

skyreflectedinmirrors
Copy link
Contributor

…& ImplicitSurfChem constructor to allow users to modfiy

Please fill in the issue number this pull request is fixing:

Fixes #608

Changes proposed in this pull request:

  • Adds user-settable integrator parameters to the InterfaceKinetics::advanceCoverages method
  • Adds the same to the Cython interface

@speth -- what would be a good test here? Something like simultaneously limiting the maximum step-size to t_out/N while limiting the maximum number of steps to N-1 would be a good test from the cython interface for those values, but I'm not sure how to test that the rtol / atol or error test failures are correctly set without lowering the tests into C++? I guess I could add some information methods (similar to those in the base Integrator class) to query these directly, but that doesn't seem to fit well with the very short lived nature of the integrator inside of InterfaceKinetics...

…& ImplicitSurfChem constructor to allow users to modfiy
@codecov
Copy link

codecov bot commented Mar 7, 2019

Codecov Report

Merging #610 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #610      +/-   ##
==========================================
+ Coverage   68.52%   68.53%   +<.01%     
==========================================
  Files         363      363              
  Lines       39957    39978      +21     
==========================================
+ Hits        27382    27400      +18     
- Misses      12575    12578       +3
Impacted Files Coverage Δ
include/cantera/kinetics/ImplicitSurfChem.h 100% <ø> (ø) ⬆️
include/cantera/kinetics/InterfaceKinetics.h 28.57% <ø> (ø) ⬆️
src/kinetics/InterfaceKinetics.cpp 74.88% <100%> (+0.17%) ⬆️
src/kinetics/ImplicitSurfChem.cpp 85.18% <100%> (+1.85%) ⬆️
src/numerics/CVodesIntegrator.cpp 76.6% <0%> (-0.92%) ⬇️
src/transport/GasTransport.cpp 90.77% <0%> (-0.21%) ⬇️

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 654e582...8634242. Read the comment docs.

@speth
Copy link
Member

speth commented Mar 7, 2019

Yes, I think your first suggested test is a good one. For the tolerances, I agree that creating
"getter" methods would be overkill. You could try a test that runs with two different sets of tolerances, and make sure that the solution is similar but not identical.

@skyreflectedinmirrors
Copy link
Contributor Author

Not sure why the OSX TravisCI job timed out?

@bryanwweber
Copy link
Member

@arghdos I restarted the job, let's see what happens

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.

There's a little bit of cleanup to be done, but this looks pretty good as far as the actual user-facing part of it goes.

interfaces/cython/cantera/test/test_reactor.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/test/test_reactor.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/test/test_reactor.py Outdated Show resolved Hide resolved
interfaces/cython/cantera/test/test_reactor.py Outdated Show resolved Hide resolved
src/kinetics/ImplicitSurfChem.cpp Outdated Show resolved Hide resolved
include/cantera/kinetics/ImplicitSurfChem.h Outdated Show resolved Hide resolved
interfaces/cython/cantera/kinetics.pyx Outdated Show resolved Hide resolved
include/cantera/kinetics/InterfaceKinetics.h Outdated Show resolved Hide resolved
src/kinetics/ImplicitSurfChem.cpp Show resolved Hide resolved
src/kinetics/InterfaceKinetics.cpp Outdated Show resolved Hide resolved
@skyreflectedinmirrors skyreflectedinmirrors force-pushed the add_interface_kinetics_integrator_options branch from fe5e75f to 995a758 Compare March 25, 2019 20:52
@skyreflectedinmirrors skyreflectedinmirrors force-pushed the add_interface_kinetics_integrator_options branch from 995a758 to 8634242 Compare April 8, 2019 14:53
@skyreflectedinmirrors
Copy link
Contributor Author

@speth -- updated this, should be good now

@speth speth merged commit 848a3bf into Cantera:master Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow setting of integrator options for InterfaceKinetics.advanceCoverages
3 participants