BUG: Fix misleading ValueError in convolve on empty inputs due to variable swapping#30277
Merged
WarrenWeckesser merged 6 commits intonumpy:mainfrom Nov 28, 2025
Merged
BUG: Fix misleading ValueError in convolve on empty inputs due to variable swapping#30277WarrenWeckesser merged 6 commits intonumpy:mainfrom
WarrenWeckesser merged 6 commits intonumpy:mainfrom
Conversation
Previously, numpy.convolve swapped arguments for optimization if the second argument was longer than the first. This occurred before checking for empty arrays. Consequently, if the first argument was empty, the swap would cause the error message to incorrectly report that the second argument was empty (e.g., "v cannot be empty"). This commit moves the validation of empty inputs to occur before the optimization swap. This ensures that the ValueError correctly identifies which argument is actually empty.
This adds a regression test to ensure `numpy.convolve` raises the correct ValueError when the first argument is empty. Previously, internal variable swapping caused the error message to incorrectly attribute the empty input to the wrong argument.
seberg
reviewed
Nov 21, 2025
Member
seberg
left a comment
There was a problem hiding this comment.
I like that the fix is to just move things, but the test is failing, mixing up the parameter names.
(You wrote the test as if the variable name matters, but the argument name matters and we are passing things positionally.)
numpy/_core/tests/test_numeric.py
Outdated
|
|
||
| with pytest.raises(TypeError, match="Input should be a NumPy array"): | ||
| np.astype(data, np.float64) | ||
| np.astype(data, np.float64) No newline at end of file |
Member
There was a problem hiding this comment.
Would be nice to not change this.
jorenham
reviewed
Nov 21, 2025
numpy/_core/numeric.py
Outdated
| raise ValueError('a cannot be empty') | ||
| if len(v) == 0: | ||
| raise ValueError('v cannot be empty') | ||
| if (len(v) > len(a)): |
Member
There was a problem hiding this comment.
Suggested change
| if (len(v) > len(a)): | |
| if len(v) > len(a): |
might as well 🤷
Contributor
Author
There was a problem hiding this comment.
Thank you for pointing this out. made the change in next commit
Refactored the regression test for `convolve` to directly use `np.array` instead of named variables, avoiding confusion about parameter names vs argument names. Also removed unnecessary brakets in numeric.py
Contributor
Author
|
@seberg I've refactored the test to pass arrays directly to avoid confusion between variable names and argument positions. Let me know if it looks okay now! |
Member
|
Lint error needs fixing, just needs the file to end with a newline. |
Contributor
Author
Just added missing new line at the end of |
Member
|
Thanks @Jongwan93. I pushed a small fix to a comment. I'll merge once the latest CI run completes. |
cakedev0
pushed a commit
to cakedev0/numpy
that referenced
this pull request
Dec 5, 2025
…iable swapping (numpy#30277) * BUG: Fix misleading ValueError in convolve on empty inputs Previously, numpy.convolve swapped arguments for optimization if the second argument was longer than the first. This occurred before checking for empty arrays. Consequently, if the first argument was empty, the swap would cause the error message to incorrectly report that the second argument was empty (e.g., "v cannot be empty").
IndifferentArea
pushed a commit
to IndifferentArea/numpy
that referenced
this pull request
Dec 7, 2025
…iable swapping (numpy#30277) * BUG: Fix misleading ValueError in convolve on empty inputs Previously, numpy.convolve swapped arguments for optimization if the second argument was longer than the first. This occurred before checking for empty arrays. Consequently, if the first argument was empty, the swap would cause the error message to incorrectly report that the second argument was empty (e.g., "v cannot be empty").
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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
Fixes #30272
What does this implement/fix?
This PR fixes a misleading error message in
numpy.convolvewhen the first argument is empty and the second is not.Previously, an internal optimization swapped the arguments (
aandv) based on their length before performing the empty check. This caused theValueErrorto incorrectly report that the second argument was empty when it was actually the first argument that was empty.This change moves the validation logic before the optimization swap.
Additional information
I have added a regression test case in
numpy/_core/tests/test_numeric.pyto ensure the correct error message is raised.