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

MAINT: dense_output and t_eval are mutually exclusive inputs #9282

Merged
merged 4 commits into from
Oct 2, 2018
Merged

MAINT: dense_output and t_eval are mutually exclusive inputs #9282

merged 4 commits into from
Oct 2, 2018

Conversation

MatthewFlamm
Copy link
Contributor

As found in #9228 in solve_ivp, if both t_eval and dense_output are used, an exception occurs when ts and interpolants lengths do not match.

Added this information to the docstring. Could consider adding an Exception with more informative message as well.

@MatthewFlamm MatthewFlamm changed the title dense_output and t_eval are mutually exclusive inputs DOC: dense_output and t_eval are mutually exclusive inputs Sep 17, 2018
@drhagen
Copy link
Contributor

drhagen commented Sep 18, 2018

I am not sure the current behavior is the correct behavior. I don't see why both of these options could not be set simultaneously. I think @nmayorov wrote this part of the code. I would like to hear his opinion.

@MatthewFlamm
Copy link
Contributor Author

I can see the reasoning behind the functionality as is. Using neither option stores t and y at every integrator step. Using only t_eval stores only t and y at t=t_eval. Using only dense_output stores t, y and an interpolating function at every integrator step. In my opinion, the reason for specifying t_eval is for convenience and for reducing memory requirements. One still gets similar convenience using dense_output by adding one more line after the solution is returned (like in the issue linked originally). What are the use cases for specifying both?

I would suggest the following options:

  1. just adding the info to the docstring as in present
  2. do 1, but also raise an exception with informative message
  3. Ignore t_eval, i.e. set to None, and print a warning that directs the user to evaluate the solution using the sol return from dense_output
  4. Keep the time steps of the solver when t_eval is set, so that the interpolating function can be generated correctly. Note however this increases the memory requirement when just t_eval is set, which is one key reason for using t_eval in the first place. I personally don't like this option.
  5. Add new functionality when both t_eval and dense_output are used.

@drhagen
Copy link
Contributor

drhagen commented Sep 19, 2018

I don't think there is a use-case for specifying both. But I think the sensible behavior when both are specified is this:

  • t_eval controls only the behavior of the ts and ys fields. When an array, ts equals t_eval; when None, ts equals the time steps chosen by the solver.
  • dense_output controls only the behavior of sol. When True, sol is and OdeSolution; when False, it is None.
  • t_events is always the same regardless of the t_eval and dense_output settings.

@MatthewFlamm
Copy link
Contributor Author

I think this is a fine option. I can take a crack, but I still haven't been able to build scipy for development, despite reading the developers build documentation several times over.

@drhagen
Copy link
Contributor

drhagen commented Sep 19, 2018

Go ahead and take a crack at it. I can't build the dev environment anymore either. I just modify the files of an installed instance in an isolated conda environment and then copy the files to the git repo once everything is working.

@MatthewFlamm
Copy link
Contributor Author

MatthewFlamm commented Sep 19, 2018 via email

@MatthewFlamm
Copy link
Contributor Author

MatthewFlamm commented Sep 19, 2018

I have tested this with the following code adapted from the issue above, where sol is the solution when t_eval and dense_output are both set, sol_teval is the solution when only t_eval is set, and sol_dense is the solution when only dense_output is set.

I think this satisfies what @drhagen suggested.

import numpy as np
from scipy.integrate import solve_ivp

def fprime(x, y):
    return 3 * x**2 + 12 * x - 4

def event(x, y):
    return y

xspan = (-8, 4)
xeval, h = np.linspace(*xspan, retstep=True)

sol = solve_ivp(fprime, xspan, np.array([-120]), t_eval=xeval, events=[event],
                dense_output=True, max_step=h)

sol_teval=solve_ivp(fprime, xspan, np.array([-120]), t_eval=xeval, events=[event],
                max_step=h)

sol_dense = solve_ivp(fprime, xspan, np.array([-120]), events=[event],
                dense_output=True, max_step=h)

print(np.all(sol.t==sol_teval.t))
print(np.all(sol.y==sol_teval.y))
print(np.all(sol.sol(xeval)==sol_dense.sol(xeval)))
print(np.all(sol.t_events[0]==sol_teval.t_events[0]))
print(np.all(sol.t_events[0]==sol_dense.t_events[0]))

Running this returns:

True
True
True
True
True

@MatthewFlamm MatthewFlamm changed the title DOC: dense_output and t_eval are mutually exclusive inputs MAINT: dense_output and t_eval are mutually exclusive inputs Sep 20, 2018
@MatthewFlamm
Copy link
Contributor Author

I added a simple test that would have failed previously but passes now (on my end). I also changed title to MAINT, since it is no longer a documentation change. Let me know if it should be another category like bug.

@drhagen
Copy link
Contributor

drhagen commented Sep 20, 2018

This seems like the correct behavior to me. Very nicely done.

@nmayorov
Copy link
Contributor

nmayorov commented Oct 2, 2018

Hi!

I think these are good suggestion and implementation. I'm merging this, quite sure nobody will mind it.

@nmayorov nmayorov merged commit f39f1d2 into scipy:master Oct 2, 2018
@ilayn ilayn added this to the 1.2.0 milestone Oct 3, 2018
@ilayn ilayn added the maintenance Items related to regular maintenance tasks label Oct 3, 2018
@MatthewFlamm MatthewFlamm deleted the ivp_docfix branch October 3, 2018 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks scipy.integrate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants