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

DOC: integrate: clarify that event and jac must have the same signature as fun #18876

Closed
jdsahr opened this issue Jul 14, 2023 · 6 comments
Closed
Labels
Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org good first issue Good topic for first contributor pull requests, with a relatively straightforward solution scipy.integrate
Milestone

Comments

@jdsahr
Copy link

jdsahr commented Jul 14, 2023

Issue with current documentation:

scipy.integrate.solve_ivp()

the doc for the the keyword argument "events" begins as follows ---

events: callable, or list of callables, optional
Events to track. If None (default), no events will be tracked. Each event occurs at the zeros of a continuous function of time and state. Each function must have the signature event(t, y) and return a float.

The signature as stated is incorrect; it should in fact be

event(t, y, *args)


I figured this out by staring at error messages along the lines of "the event signature has 2 arguments; 4 were given")

The extra arguments were being supplied internally by solve_ivp()

Idea or request for content:

Two proposed solutions:

(1) update the docs to show the proper signature, or

(2) In principle you could detect whether or not the *args had been included (and add them if they weren't), but that just seems like asking for trouble somehow; for example, you'd have to modify the source code and then test the bejeezus out of it.

I'd recommend (1), because it won't break any existing user's code, doesn't touch the source code, and will help new users use the existing code correctly.


The reason that *args are needed is that solve_ivp() is going to tiptoe through a region where an event occurred, probably calling itself with a reduced time-step or custom t_eval array. It's going to need *args to do that. Alternatively, solve_ivp() will do some sort of interpolation fun and games (e.g. dense_output=True) with a zero-finder. If the latter is true, then the existing signature would be correct ... but it obviously isn't correct.

On the other hand, one could argue that the interpolation approach is simpler (and superior) to the "tiptoe" approach, in which case the *args shouldn't be necessary, and the original signature correct.

If this was an initial offering of solve_ivp then I'd recommend solution (3), but solve_ivp has been out in the world for a while, so better to just update the docs. It won't hurt any code which doesn't have *args.

Additional context (e.g. screenshots, GIFs)

No response

@jdsahr jdsahr added the Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org label Jul 14, 2023
@roryyorke
Copy link
Contributor

roryyorke commented Jul 15, 2023

Could you give a minimal example showing the problem, and also the Scipy version?

The below, which has an event function without *args, works with Scipy 1.10.1, Numpy 1.24.3, Python 3.10.11, running on an Ubuntu 20.04 x64 systems:

# ball falling under gravity with friction; terminate when y = 0

import matplotlib.pyplot as plt

import numpy as np
from scipy.integrate import solve_ivp

def fallde(t, y):
    pos, vel = y
    acc = -9.8 - 0.1*vel
    return np.array([vel,acc])

def hitground(t, y):
    return y[0]

hitground.terminal = True

sol = solve_ivp(fallde, [0, 10], [10, 0], events=[hitground])

plt.plot(sol.t, sol.y.T, '-x')

Resulting figure:

image

@jdsahr
Copy link
Author

jdsahr commented Jul 15, 2023 via email

@rkern
Copy link
Member

rkern commented Jul 15, 2023

import scipy
print(scipy.__version__)

@jdsahr
Copy link
Author

jdsahr commented Jul 15, 2023 via email

@roryyorke
Copy link
Contributor

I see: it's because you're using the args parameter to solve_ivp. These extra arguments are passed not only to fun (the DE function), but also events and jac. This is documented with the args parameter (see below and in the on-line docs ), but it is a little confusing (I certainly didn't realize this when trying to reproduce your problem), and there could be a reference to args in the definitions of the fun, events, and jac parameters ("see also args", or "if specified, args is also passed to this function", etc.)

args : tuple, optional

Additional arguments to pass to the user-defined functions. If given, the additional arguments are passed to all user-defined functions. So if, for example, fun has the signature fun(t, y, a, b, c), then jac (if given) and any event functions must have the same signature, and args must be a tuple of length 3.

@j-bowhay j-bowhay added scipy.integrate good first issue Good topic for first contributor pull requests, with a relatively straightforward solution labels Jul 16, 2023
@j-bowhay j-bowhay changed the title DOC: <Please write a comprehensive title after the 'DOC: ' prefix> DOC: integrate: clarify that event and jac must have the same signature as fun Jul 16, 2023
@jdsahr
Copy link
Author

jdsahr commented Jul 16, 2023 via email

gertingold added a commit to gertingold/scipy that referenced this issue Aug 18, 2023
gertingold added a commit to gertingold/scipy that referenced this issue Aug 18, 2023
j-bowhay pushed a commit that referenced this issue Aug 19, 2023
* DOC: add comment on args in solve_ivp (#18876)

[skip ci]

* DOC: make reference to args more explicit

[skip ci]
@j-bowhay j-bowhay added this to the 1.12.0 milestone Aug 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues related to the SciPy documentation. Also check https://github.com/scipy/scipy.org good first issue Good topic for first contributor pull requests, with a relatively straightforward solution scipy.integrate
Projects
None yet
Development

No branches or pull requests

4 participants