OrdinaryDiffEq.jl icon indicating copy to clipboard operation
OrdinaryDiffEq.jl copied to clipboard

Redesign default ODE solver to be type-grounded and lazy

Open oscardssmith opened this issue 9 months ago • 9 comments

This is the version of https://github.com/SciML/OrdinaryDiffEq.jl/pull/2103 that doesn't end up initializing the integrator caches that are unused. The code is fairly janky, but it all works and is type stable.

oscardssmith avatar May 08 '24 16:05 oscardssmith

Seems like there was a rebase issue in the cache builds

ChrisRackauckas avatar May 09 '24 13:05 ChrisRackauckas

I believe the rebase is now actually correct. Rebasing across the formatting changes is really annoying.

oscardssmith avatar May 09 '24 18:05 oscardssmith

@ChrisRackauckas I think your merge was wrong. I'll fix it.

oscardssmith avatar May 14 '24 00:05 oscardssmith

Awesome, looks like tests pass sans codecov nonsense. Can you prepare the two big downstream changes as well, the fix to DelayDiffEq and also a change to DifferentialEquations.jl which uses this as the default?

ChrisRackauckas avatar May 14 '24 14:05 ChrisRackauckas

I think I can make this work without requiring a DelayDiffEq fix (current just needs to initialize to 0). https://github.com/SciML/OrdinaryDiffEq.jl/pull/2185 has all the changes needed for this to be used by default. Want me to merge it in with this PR? The DifferentialEquations side doesn't actually need any changes. #2185 is more specific, so it will automatically do the right thing.

oscardssmith avatar May 14 '24 14:05 oscardssmith

Yes 1 PR, I'm a bit confused which one is the one to look at.

ChrisRackauckas avatar May 14 '24 14:05 ChrisRackauckas

Can you port something similar to https://github.com/SciML/DifferentialEquations.jl/blob/master/test/default_ode_alg_test.jl over to here so it's tested? Big feature to not have a test for 😅

ChrisRackauckas avatar May 14 '24 15:05 ChrisRackauckas

Unfortunately the fully grounded approach makes this type of test impossible. I think the only thing we can test is that the number of steps and f evals roughly match what we would expect. These tests have now been added.

oscardssmith avatar May 14 '24 19:05 oscardssmith

You can check the alg_choice which is tracked?

ChrisRackauckas avatar May 14 '24 19:05 ChrisRackauckas

I think this now all works.

oscardssmith avatar May 15 '24 19:05 oscardssmith

I wonder if it would be beneficial to test the accuracy of the chosen algorithms by the analytic solution. (if I'm not mistaken, we expect that there be a relation between the tolerance and the solution error) It can be found in https://github.com/SciML/DiffEqProblemLibrary.jl/blob/8d2cf0c543a734053a339cb1eed2e27fe512b04b/lib/ODEProblemLibrary/src/ode_linear_prob.jl#L1-L21

What if we use the problems that defined in ODEProblemLibrary?

prbzrg avatar May 15 '24 21:05 prbzrg

we could, but I'm not sure that's the best test. figuring out appropriate error levels there is always a little ad hoc, and I'm this case, we know exactly what the solution we expect is (it should just be the same as if we pick the solver ourself)

oscardssmith avatar May 15 '24 21:05 oscardssmith

I believe this is now ready to merge.

oscardssmith avatar May 20 '24 16:05 oscardssmith

Test fails

ChrisRackauckas avatar May 20 '24 16:05 ChrisRackauckas

Before merging this, I think the issues linked to https://github.com/SciML/OrdinaryDiffEq.jl/pull/2103 should be checked and specify if this PR can resolve them.

prbzrg avatar May 20 '24 17:05 prbzrg

This I believe does all the things that PR requires. Specifically, it is type stable, and roughly the same for precompile. I'm not sure either version actually makes a significant precompile difference, since they both take us from precompiling 6 solvers to a single solver that calls all the functions that those 6 solvers would have called.

oscardssmith avatar May 20 '24 19:05 oscardssmith

What's the test failure from?

ChrisRackauckas avatar May 20 '24 19:05 ChrisRackauckas

The test failure was from me being wrong about which algorithm would be used for exrober at the last timestep. (I think due to the same issue that causes https://github.com/SciML/OrdinaryDiffEq.jl/issues/2198). The test has been adjusted.

oscardssmith avatar May 20 '24 20:05 oscardssmith