BUG: Fix matvec/vecmat in-place aliasing (out=input produces zeros)#31150
Conversation
Clear NPY_ITER_OVERLAP_ASSUME_ELEMENTWISE on the output operand of matvec and vecmat, matching the existing matmul fix. Without this, the iterator skips the overlap copy when out aliases an input, and BLAS gemv clobbers the input buffer. Closes numpygh-31144 Made-with: Cursor
jorenham
left a comment
There was a problem hiding this comment.
Thanks for your contribution.
Could you revert the file permission changes?
Made-with: Cursor
Done. I hope everything was done properly 😄 |
| expected = np.vecmat(b, a) | ||
| c = np.vecmat(b, a, out=b) | ||
| assert_(c is b) | ||
| assert_equal(c, expected) |
There was a problem hiding this comment.
Both tests do indeed fail before and pass after the patch.
Note for other NumPy devs--the original problem was only reproducible for me on x86_64 Linux with OpenBLAS, not with ARM Mac/Accelerate, in case that's relevant.
| expected = np.matvec(a, b) | ||
| c = np.matvec(a, b, out=b) | ||
| assert_(c is b) | ||
| assert_equal(c, expected) |
There was a problem hiding this comment.
I suppose we could try to parametrize the two new tests, but the complexity of the argument reversal probably negates any potential simplification.
I guess these are both very similar to the original test_matmul_out() above.
Minor/debatable points might be preference for assert over the old assert_ that should no longer be needed in new tests, and maybe assert_allclose given we're comparing floats here (bit of a stretch that it matters, but if they do or could someday follow different codepaths...).
There was a problem hiding this comment.
happy to switch to assert and assert_allclose if you think that might be better. I was (like you mentioned) just matching the style of test_matmul_out
There was a problem hiding this comment.
should I make the edits or leave as is? im okay with either
There was a problem hiding this comment.
You could for example parametrize it with a lambda A, x: np.vecmat(A, x) and lambda A, x: np.matvec(x, A).
I would also prefer if you move these to test_ufunc.py, there seem to be more tests for these there, and that file is at least slightly smaller.
But otherwise this looks good and really we could basically put it in as is.
There was a problem hiding this comment.
should be done now, thanks for the quick feedback
Addresses reviewer feedback on numpygh-31150: - Remove test_matvec_out/test_vecmat_out from test_multiarray.py - Add parametrized test_matvec_vecmat_out in test_ufunc.py using lambdas - Switch to bare assert and assert_allclose Made-with: Cursor
Made-with: Cursor
seberg
left a comment
There was a problem hiding this comment.
Thanks, LGTM now and an important fix if someone uses in-place operators here.
(Just waiting for CI.)
…umpy#31150) `np.matvec(A, b, out=b)` returns all zeros because matvec and vecmat are missing the ~NPY_ITER_OVERLAP_ASSUME_ELEMENTWISE output flag that matmul already has. Without it the iterator ends up skipping the overlap check when out is the same array as an input so no temporary copy is made and BLAS gemv clobbers the input buffer. This PR adds the same fix to matvec and vecmat and adds regression testing for both.
BUG: Fix matvec/vecmat in-place aliasing (out=input produces zeros) (#31150)
PR summary
Fixes #31144.
np.matvec(A, b, out=b)returns all zeros becausematvecandvecmatare missing the~NPY_ITER_OVERLAP_ASSUME_ELEMENTWISEoutput flag thatmatmulalready has. Without it the iterator ends up skipping the overlap check whenoutis the same array as an input so no temporary copy is made and BLASgemvclobbers the input buffer.This PR adds the same fix to
matvecandvecmatand adds regression testing for both.First time committer introduction
Hi, I'm Ijtihed Kilani (ijtihed.com); I'm an early-career software engineer based in Helsinki, Finland. I work mainly on physics-informed simulations for robotics and autonomy, currently at Supercell working on new games. I've used NumPy heavily for linear algebra in simulation code/studies.
AI Disclosure
This fix was made with help from Claude in Cursor. I used it to look over any assumptions I might have missed and draft the basic code changes which I then looked over, then rewrote and verified. All changes were reviewed and verified by me before submission.
Made with Cursor