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

Fix for Sundials 6.6 behavior changes #1570

Merged
merged 6 commits into from Aug 4, 2023
Merged

Conversation

speth
Copy link
Member

@speth speth commented Aug 4, 2023

Changes proposed in this pull request

  • Avoid using SUNDIALS with the CV_NORMAL / IDA_NORMAL integration option which has a breaking change in behavior starting with SUNDIALS 6.6.
  • Provide better error messages for some SUNDIALS errors
  • Mark Reactor and ReactorNet methods as const where possible
  • Fix getting sensitivities after calling ReactorNet.advance to use interpolation correctly

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

Resolves #1554
Resolves #1195

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

Do the interpolation required for "advance" explicitly, rather
than relying on SUNDIALS to do it as part of the call to CVode() or
IDASolve().

This circumvents a change in behavior introduced in SUNDIALS 6.6.

Fixes Cantera#1554.
Include the name of the return code flag, not just its numeric value,
and the CVODES-generated error message for all errors.
@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #1570 (652ed24) into main (6ba497c) will increase coverage by 0.05%.
Report is 3 commits behind head on main.
The diff coverage is 84.14%.

@@            Coverage Diff             @@
##             main    #1570      +/-   ##
==========================================
+ Coverage   70.45%   70.51%   +0.05%     
==========================================
  Files         379      379              
  Lines       59093    59112      +19     
  Branches    21230    21232       +2     
==========================================
+ Hits        41636    41682      +46     
+ Misses      14376    14354      -22     
+ Partials     3081     3076       -5     
Files Changed Coverage Δ
include/cantera/numerics/CVodesIntegrator.h 31.25% <ø> (ø)
include/cantera/numerics/IdasIntegrator.h 0.00% <ø> (ø)
src/zeroD/Reactor.cpp 85.47% <50.00%> (ø)
src/numerics/CVodesIntegrator.cpp 71.65% <75.55%> (+5.71%) ⬆️
src/numerics/IdasIntegrator.cpp 72.16% <95.83%> (+1.41%) ⬆️
include/cantera/kinetics/ImplicitSurfChem.h 100.00% <100.00%> (ø)
include/cantera/numerics/FuncEval.h 42.85% <100.00%> (ø)
include/cantera/zeroD/Reactor.h 61.76% <100.00%> (ø)
include/cantera/zeroD/ReactorNet.h 76.66% <100.00%> (ø)
src/zeroD/ReactorNet.cpp 81.21% <100.00%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Previously, values would be returned for the sensitivity at the
last internal timestep of the integrator, rather than being interpolated
to the user-specified integration time.
@speth speth marked this pull request as ready for review August 4, 2023 19:41
Copy link
Member

@ischoegl ischoegl left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ischoegl ischoegl merged commit e0a2f11 into Cantera:main Aug 4, 2023
42 checks passed
@ischoegl ischoegl mentioned this pull request Aug 4, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Breaking change in Sundials 6.6 Python ReactorNet advance function providing convoluted error message
2 participants