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

[WIP] Use new solve_ivp for ODE integration. #110

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ixjlyons
Copy link
Collaborator

Initial working implementation in place. Some notes:

  • scipy 1.0 isn't available via conda-forge yet
  • All the integrators seem to be either super slow or unstable for the CoulombPendulumSystem if you integrate out beyond the sticking point for much time. The fixed time step Runge-Kutta solver we're using currently is nice there though it has accuracy issues. Could just bring that back I suppose.
  • Currently the integration method is specified via an attribute on the system class. It should probably at least be a property with a setter that raises an ValueError if you try to set it to something not available.
  • I think the ode_func is intended to always be vectorized, but I'm guessing some fun traceback will result if it's not. Should probably handle that by passing in dummy values when it's set.

Fixes #102

@moorepants
Copy link
Owner

Cool, thanks for adding. The new integrators are written in Python so they will be slow. After reading a bit about them, I'm not sure that was a great move on SciPy's part. I like the new unified interface, but why choose not to rely on battle hardened numerical codes doesn't make much sense.

I think I should rename ode_func to something more generic like diff_func or differential_equation_func, so it doesn't limit us to ODEs in the long run.

If you want to install SciPy 1.0 via pip wheels on the server we can, but I'd prefer to try to stick with conda based stuff on defaults or the forge.

I like the idea of a check in the setter for ode_func to make sure it can work with 1d arrays as arguments.

@@ -11,7 +11,7 @@ dependencies:
- coverage
- sphinx
- numpy
- scipy
- scipy>=1.0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setup.py also needs this designation


return int_res[:, 0], int_res[:, 1], res[1, :]
return int_res[0], int_res[1], res[1, :]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I'm not sure it was a good idea to swith x,t in the input and spit out the transpose wrt to odeint. There is so much code in the wild that uses the other pattern.

t_eval=times,
method=integrator,
vectorized=True,
**kwargs)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I want to switch to just using this new method. I like the idea of having the design able to support any kind of integrators (e.g. sundials, scipy variety, scikit odes, etc). The previous design was setup so you write a single method for any new integrator that you may want to add.

return sp.integrate.solve_ivp(self._array_rhs_eval_func,
(times[0], times[-1]),
initial_conditions,
t_eval=times,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this was a good API decision either, why do yo uhave to pass in t0, tf, and t (an array)?

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.

Use the new ode solvers in scipy 1.0
2 participants