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: avoid a segfault when calling astropy.convolution.convolve on an empty array #15840
BUG: avoid a segfault when calling astropy.convolution.convolve on an empty array #15840
Conversation
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
👋 Thank you for your draft pull request! Do you know that you can use |
81f5871
to
11dd8d0
Compare
astropy/convolution/convolve.py
Outdated
@@ -296,6 +296,8 @@ def convolve( | |||
) | |||
elif array_internal.ndim != kernel_internal.ndim: | |||
raise Exception("array and kernel have differing number of dimensions.") | |||
elif array_internal.size == 0: | |||
raise Exception("cannot convolve empty array") |
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.
raising the base Exception
type isn't recommended (ValueError
would be more appropriate here I think). It should be possible to refine these exceptions to a more specialised type without breaking user code that might rely on current behaviour since ValueError
is a subclass of Exception
, so any code that would already catch Exception
would also catch ValueError
.
I'll leave it to reviewers to decide whether such a change is desirable or not and wether it's in scope to this PR or better left as a follow up.
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.
👍 on replacing with a ValueError
in both cases here.
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.
thanks for your feedback ! I'll give it a couple days to see if it raises any eyebrows and if not, I'll push the change !
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.
no complaints in 2 days -> pushed !
astropy/convolution/convolve.py
Outdated
@@ -296,6 +296,8 @@ def convolve( | |||
) | |||
elif array_internal.ndim != kernel_internal.ndim: | |||
raise Exception("array and kernel have differing number of dimensions.") | |||
elif array_internal.size == 0: | |||
raise Exception("cannot convolve empty array") |
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.
👍 on replacing with a ValueError
in both cases here.
Going to wait a bit in case @astrofrog has comments. Thanks! |
@@ -289,13 +289,15 @@ def convolve( | |||
|
|||
# Check dimensionality | |||
if array_internal.ndim == 0: | |||
raise Exception("cannot convolve 0-dimensional arrays") | |||
raise ValueError("cannot convolve 0-dimensional arrays") |
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.
Does change the exception type, so maybe better not backport.
Other than that, since I haven't heard from Tom R but there is an approval, I am just going to merge.
Thanks!
Description
Fixes #10048