search

tBuLi / symfit

Symbolic Fitting; fitting as it should be.
http://symfit.readthedocs.org
MIT License
234 stars 17 forks source link

Switch out odeint for the more modern solve_ivp #343

Closed pckroon closed 2 years ago

pckroon commented 2 years ago

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.

tBuLi commented 2 years ago

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 commented 2 years ago

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?