SciML / OrdinaryDiffEq.jl

High performance ordinary differential equation (ODE) and differential-algebraic equation (DAE) solvers, including neural ordinary differential equations (neural ODEs) and scientific machine learning (SciML)
https://diffeq.sciml.ai/latest/
Other
521 stars 198 forks source link

Redesign default ODE solver to be type-grounded and lazy #2184

Closed oscardssmith closed 1 month ago

oscardssmith commented 2 months ago

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.

ChrisRackauckas commented 2 months ago

Seems like there was a rebase issue in the cache builds

oscardssmith commented 2 months ago

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

oscardssmith commented 1 month ago

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

ChrisRackauckas commented 1 month ago

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?

oscardssmith commented 1 month ago

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.

ChrisRackauckas commented 1 month ago

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

ChrisRackauckas commented 1 month ago

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 😅

oscardssmith commented 1 month ago

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.

ChrisRackauckas commented 1 month ago

You can check the alg_choice which is tracked?

oscardssmith commented 1 month ago

I think this now all works.

prbzrg commented 1 month ago

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?

oscardssmith commented 1 month ago

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 commented 1 month ago

I believe this is now ready to merge.

ChrisRackauckas commented 1 month ago

Test fails

prbzrg commented 1 month ago

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.

oscardssmith commented 1 month ago

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.

ChrisRackauckas commented 1 month ago

What's the test failure from?

oscardssmith commented 1 month ago

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.