-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
BUG interp wrong if input xp isn't sorted #10448
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
Comments
The docstring explicitly states:
The description is accurate: sorting does happen if a period is specified:
But perhaps we should make |
I'm not a huge fan of this, as it adds runtime to Essentially, either your data is:
Is it possible to trivially check in |
@eric-wieser - the only reason I suggested it is that it would be a very cheap addition (to |
Good points. Another option would be to raise an exception is the input data is not sorted as expected. The check numpy.all(a[1:] > a[:-1]) is pretty cheap. |
I'd concur that this is a big problem. I don't recall that
Thankfully this isn't a subtle bug... this time. Throughout this process I probably read the documentation in ipython 2-4 times via
Even a simple assert that I don't believe many people think of an interpolant as requiring a monotonic increase, e.g., https://www.mathworks.com/help/matlab/ref/interp1.html, so requiring this implicitly without any warning when there is an error (which could produce a very, very nasty bug) is dangerous to the community at large using this function. If runtime is this important, perhaps the function should be renamed to Perhaps something an edit to the docstring would be good from the current
to
|
@eric-wieser, thanks in advance for your consideration of this request to clarify |
Most simply, I'd recommend moving
from the notes to the main doc string so this is one of the first things folks see. This won't affect runtime and will get the key message across clearly. |
So I ran into (nearly) this problem and it caused me headaches to find out what was causing the unexpected behaviour. It turned out I provided a monotonically increasing (but not strictly) MWE
Output
I honestly would have expected to get an error on this from |
@euronion - it is not surprising the code doesn't know what to do if you try to interpolate through a bi-valued function... Still, since equal-valued is such a corner case, adding "strictly" to the documentation is not a bad idea - could you perhaps make a quick PR? The github interface should suffice for this (relevant file is |
Adding 'strictly' to make the conditions to be met by the `xp` argument even more clear. Following the suggestion in numpy#10448 (comment) .
@mhvk - Sure thing. I hope I did not violate to many guidelines for commits and PRs . Regarding the topic: |
THanks, note the PR must be to the numpy branch! On topic: I quite strongly prefer not checking (the check is not cheap compared to the operation), but can see an argument for at least having an auto-sort option. Indeed, |
My mistake, sorry.
Will keep this in mind, thanks for pointing it out. |
Note that the result is correct under the assumption that you are trying to interpolate a right-continuous function, see also my comment at scipy/scipy#9886 (comment). |
It seems that there is hesitation to provide checking for this very insidious behavior because of impact on runtime. Could checking for increasing monotonicity not be enabled by an argument? I feel that the vast majority of use cases, the bugs produced by this behavior is more detrimental to the runtime hit, but if runtime is of critical importance to the use case, one could disable checking by passing in a simple |
The docs are pretty clear about this now. The first line reads:
And the
Then there's also an explicit warning:
So this behavior is explicitly mentioned in three different places, making it pretty hard to miss.
If you don't know if your data is sorted, then why not just use something like |
Yes I am aware that the docs call it out, but users are not looking at the docs every single time they use the function. A frequent use of The fact that other interpolating methods that require strict increasing monotonicity do this check by default, e.g. |
Adding an optional keyword argument requires you to be aware of monotonicity requirement too, so that won't solve this problem. But like I said before, if your data might not be sorted, then you should probably just sort it. But if you don't want to that for whatever reason, then I'd suggest using an assert np.all(np.sort(xp, stable=True) == xp) I would consider this to be more idiomatic than the keyword argument you suggested. |
Agreed with @jorenham - also because there are more than a few cases where sorting the data just gives a different wrong answer (e.g., if xp, yp are smooth but non-monotonic - sorting then definitely won't help, you would actually have to specify the branch). |
Well I would've suggested the check occur by default as it is done with other packages, but I know that would be a breaking change. Fine, just wanted to voice my opinion and agree with the others earlier in this thread who voiced similar sentiments. I see that this is not a popular one among the maintainers of numpy. |
By the way, I do think this issue has become more relevant as Scipy has moved its Furthermore their new page on 1D interpolation with an example of using |
MWE:
The text was updated successfully, but these errors were encountered: