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

Switch out odeint for the more modern solve_ivp #343

Merged
merged 8 commits into from
Apr 26, 2022

Conversation

pckroon
Copy link
Collaborator

@pckroon pckroon commented Oct 28, 2021

This PR switches the ODEModel from the old odeint to the newer solve_ivp. And there was much rejoicing. Also does some minor polishing left and right.

This is needed for test_initial_parameters: if the LSODA integrator (our default option) has to take too small steps in t with the default values of rtol/atol (1e-3/1e-6) it doesn't always converge...
@pckroon pckroon added the hacktoberfest-accepted Accepted Hacktoberfest contribution label Nov 2, 2021
Georg Schlisio and others added 5 commits April 8, 2022 18:49
* Fix pickle errors

- Provide Argument.__setstate__. This used to be provided by sympy
- Make sure super().__getstate__ always returns a dict, so we can add our own attributes

* Address deprecation of np.float

* Fix numerical issue in test_harmonic_oscillator_errors
@tBuLi
Copy link
Owner

tBuLi commented Apr 8, 2022

Almost all failing tests have been resolved by setting the atol/rtol LSODA settings back to the original settings of odeint. (I've also rebased on he current master branch) Furthermore I made some performance improvements by not repeating the generation of the system of equations every iteration, but instead turned that into a cached_property.

The range also does not seem to need +- 1e-8 or something similar, works just fine for all our tests without it (and according to the documentation) so not sure why this was needed?

There are only two tests left that fail: pickling and parameters as initial guesses. Pickling seems to be simple to fix: this is due to the fact that we no longer support positional arguments for the ODE solver, which seems like a good thing to me so we can just change the test. The latter test is a bit more complicated: LSODA throws an error which seems to be due to loss of precission. But this is probably also solvable by identifying the changed standard settings as compared to odeint which lead to this problem.

@pckroon
Copy link
Collaborator Author

pckroon commented Apr 11, 2022

Almost all failing tests have been resolved by setting the atol/rtol LSODA settings back to the original settings of odeint. (I've also rebased on he current master branch) Furthermore I made some performance improvements by not repeating the generation of the system of equations every iteration, but instead turned that into a cached_property.

Good finds :)

The range also does not seem to need +- 1e-8 or something similar, works just fine for all our tests without it (and according to the documentation) so not sure why this was needed?

This used to break when I tried it (I was playing with #339 at the time). Maybe scipy got updated in the meantime?

@pckroon pckroon requested a review from tBuLi April 11, 2022 13:07
@tBuLi tBuLi merged commit 2dada60 into tBuLi:master Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accepted Hacktoberfest contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants