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

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

Merged
merged 32 commits into from
May 20, 2024

Conversation

oscardssmith
Copy link
Contributor

This is the version of #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 oscardssmith force-pushed the os/default_solver-v2 branch 3 times, most recently from 929f41d to 817d250 Compare May 8, 2024 21:06
@ChrisRackauckas
Copy link
Member

Seems like there was a rebase issue in the cache builds

@oscardssmith
Copy link
Contributor Author

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

@oscardssmith
Copy link
Contributor Author

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

ChrisRackauckas and others added 6 commits May 14, 2024 09:29
This accomplishes a few things:

* Faster precompile times by precompiling less
* Full inference of results when using the automatic algorithm
* Hopefully faster load times by also precompiling less

This is done the same way as

* linearsolve SciML/LinearSolve.jl#307
* nonlinearsolve SciML/NonlinearSolve.jl#238

and is thus the more modern SciML way of doing it. It avoids dispatch by having a single algorithm that always generates the full cache and instead of dispatching between algorithms always branches for the choice.

It turns out, the mechanism already existed for this in OrdinaryDiffEq... it's CompositeAlgorithm, the same bones as AutoSwitch! As such, this reuses quite a bit of code from the auto-switch algorithms but instead of just having two choices it (currently) has 6 that it chooses between. This means that it has stiffness detection and switching behavior, but also in a size-dependent way.

There are still some optimizations to do though. Like LinearSolve.jl, it would be more efficient to have a way to initialize the caches to size zero and then have a way to re-initialize them to the correct size. Right now, it'll generate the same Jacobian N times and it shouldn't need to do that.

fix typo / test

fix precompilation choices

Update src/composite_algs.jl

Co-authored-by: Nathanael Bosch <nathanael.bosch@uni-tuebingen.de>

Update src/composite_algs.jl

switch CompositeCache away from tuple so it can start undef

Default Cache

fix precompile

remove fallbacks

remove fallbacks
@oscardssmith oscardssmith force-pushed the os/default_solver-v2 branch from e080f75 to 5f19a80 Compare May 14, 2024 13:30
@ChrisRackauckas
Copy link
Member

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
Copy link
Contributor Author

oscardssmith commented May 14, 2024

I think I can make this work without requiring a DelayDiffEq fix (current just needs to initialize to 0). #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
Copy link
Member

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

src/solve.jl Outdated Show resolved Hide resolved
@ChrisRackauckas
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

You can check the alg_choice which is tracked?

src/solve.jl Outdated Show resolved Hide resolved
src/solve.jl Outdated Show resolved Hide resolved
src/solve.jl Outdated Show resolved Hide resolved
src/solve.jl Outdated Show resolved Hide resolved
src/solve.jl Outdated Show resolved Hide resolved
@oscardssmith
Copy link
Contributor Author

I believe this is now ready to merge.

@ChrisRackauckas
Copy link
Member

Test fails

@prbzrg
Copy link
Member

prbzrg commented May 20, 2024

Before merging this, I think the issues linked to #2103 should be checked and specify if this PR can resolve them.

@oscardssmith
Copy link
Contributor Author

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
Copy link
Member

What's the test failure from?

@oscardssmith
Copy link
Contributor Author

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 #2198). The test has been adjusted.

@ChrisRackauckas ChrisRackauckas merged commit b69b4ca into SciML:master May 20, 2024
15 of 32 checks passed
@oscardssmith oscardssmith deleted the os/default_solver-v2 branch May 20, 2024 20:41
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.

3 participants