Skip to content

Conversation

@oscardssmith
Copy link
Member

This fixes #292. It turns out we were asking for interpolated derivatives for essentially no reason (and doing so caused problems whenever you hit callbacks that re-init the problem). As a nice side-benefit, this removes the 200 warnings from the test-suite.

…rivatives for essentially no reason (and doing so caused problems whenever you hit callbacks). As a nice side-benefit, this removes the 200 warnings
@ChrisRackauckas
Copy link
Member

Given the aforementioned issue, it would be good to have a test of the interpolation accuracy here. Maybe adapt one from https://github.com/SciML/OrdinaryDiffEq.jl/blob/master/test/regression/ode_dense_tests.jl and use that to test the interpolation accuracy. Show the result before and after this PR, and then with the if statement in. With exactly this PR, the interpolation accuracy should be substantially worse (and if you plot it you should show weird oscillations), but the good thing here would be to then use that to set a regression test limit that will ensure this issue isn't accidentally added in.

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Merging #408 (cae6324) into master (090c2d4) will increase coverage by 0.11%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master     #408      +/-   ##
==========================================
+ Coverage   81.32%   81.44%   +0.11%     
==========================================
  Files          11       11              
  Lines        1435     1428       -7     
==========================================
- Hits         1167     1163       -4     
+ Misses        268      265       -3     
Impacted Files Coverage Δ
src/common_interface/integrator_utils.jl 86.20% <83.33%> (+1.33%) ⬆️
src/common_interface/solve.jl 82.64% <90.90%> (+0.13%) ⬆️

... and 1 file with indirect coverage changes

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

@oscardssmith
Copy link
Member Author

I'm not seeing any problems here:
I'm using prob_ode_pollution, and, for the PR with CVODE_BDF, (reltol=1e-2 to make things worse). With the PR, I get
image
On master I get
image

To me it looks like this isn't doing anything wrong, and the results look approximately identical. prob_ode_linear didn't show any problems. Do you have a specific problem in mind that you think would show the problems because I can't find them.

@oscardssmith
Copy link
Member Author

oscardssmith commented Jun 27, 2023

I've added a set of interpolation tests and they behave very similarly with this PR and master. I had to change the test a decent amount from the OrdinaryDiffEq version though because sundials integrators can not have their adaptivity turned off.

@staticfloat
Copy link
Contributor

Something is going crazy in the formatting here.

@ChrisRackauckas ChrisRackauckas merged commit f86eea7 into SciML:master Jul 1, 2023
@oscardssmith oscardssmith deleted the fix-warnings branch July 1, 2023 15:35
@oscardssmith
Copy link
Member Author

thanks!

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.

"[IDAS ERROR] IDAGetDky Illegal value for k." with event callbacks

3 participants