-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
BUG: integrate: Fix crash with repeated t values in odeint. #8822
Conversation
|
||
dt = np.diff(t) | ||
if not((dt >= 0).all() or (dt <= 0).all()): | ||
raise ValueError("The values in t must be monotonically increasing " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, since we allow for repeated values, this should read as must either all nondecreasing or all nonincreasing but not a mixture.
or something along those lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By every definition that I have seen, the statement "t is monotonically increasing" means t[i] <= t[i+1] for all i, so repeated values are allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I guess it wouldn't hurt to state explicitly that repeated values are allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also thought that "monotonically increasing" was <
and "monotonically nondecreasing was <=
but upon reading the definitions this does indeed appear mistaken! I agree adding a (t[i] <= t[i+1] for all i)
sort of parenthetical is worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a note to the docstring and the error message about allowing repeated values.
Looks good to me apart from a tiny detail. |
Two changes to odeint: * Require `t` to be monotonically increasing or monotonically decreasing. Repeated values are allowed. * If the initial value in `t` is repeated, don't bother calling LSODA for those times. Just copy the initial condition to the output. This fixes the problem where an input such as `t = np.zeros(10)` would abort the Python interpreter (because of how the Fortran subroutine LSODA handled the repeated calls with the same value of t). Closes scipygh-8217.
af7d9e1
to
0ec9261
Compare
Thanks, issues seemed addressed. |
Two changes to odeint:
t
to be monotonically increasing or monotonicallydecreasing. Repeated values are allowed.
t
is repeated, don't bother callingLSODA for those times. Just copy the initial condition to the
output.
This fixes the problem where an input such as
t = np.zeros(10)
would abort the Python interpreter (because of how the Fortran
subroutine LSODA handled the repeated calls with the same value
of t).
Closes gh-8217.