BUG: interpolate: do not segfault on bad boundary conditions #20732
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ev-br
added
defect
j-bowhay
reviewed
May 18, 2024
ev-br
commented
May 18, 2024
j-bowhay
reviewed
May 18, 2024
Comment on lines
1352
to
1360
| l, r = [(1, 0)], [(6, 0)] # 6th derivative = 0 at x[-1] for k=3 | ||
| with assert_raises(ValueError): | ||
| # cannot fix 6th derivative at x[-1]: does not segfault | ||
| make_interp_spline(x, y, bc_type=(l, r)) | ||
|
|
||
| l, r = [(1, 0)], [(-6, 0)] # derivative order < 0 at x[-1] | ||
| with assert_raises(ValueError): | ||
| # does not segfault | ||
| make_interp_spline(x, y, bc_type=(l, r)) |
There was a problem hiding this comment.
Sorry I should have put this with my original comment but could we not make one of the bad BCs on the left so we get coverage of both branches? Also if there is an edit to be made can we add match=..... too
There was a problem hiding this comment.
Okay, the left-hand side boundary condition it is. re match=... feel free to add if you think it's essential. I'm on record that they are not.
ev-br
commented
May 18, 2024
j-bowhay
reviewed
May 18, 2024
j-bowhay
approved these changes
May 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reference issue
What does this implement/fix?
Avoid a segfault when
make_interp_splinereceives too high derivative order for a boundary condition.The underlying issue is that the input falls through all the way down to a C routine, https://github.com/scipy/scipy/blob/main/scipy/interpolate/src/__fitpack.h#L48
and triggers a UB. (in that line,
k=3is the spline order, andmis the user input)Additional information
CubicSpline explicitly checks that the boundary conditions only feature 1st or 2nd derivatives.
Checks added in this PR are likely on a loose side: orders 0 and k currently raise a ValueError for a different reason; fixing these would a nice enhancement; this PR is a pure bug fix.
found while looking at https://stackoverflow.com/questions/78482220/fixing-boundary-values-on-a-spline;