BUG: Fix buffered iterator stride after removing multi-index#31985
Open
snoopuppy582 wants to merge 1 commit into
Open
BUG: Fix buffered iterator stride after removing multi-index#31985snoopuppy582 wants to merge 1 commit into
snoopuppy582 wants to merge 1 commit into
Conversation
| if ((itflags & NPY_ITFLAG_HASMULTIINDEX) && | ||
| NAD_SHAPE(axisdata) == 1 && op_stride == 0) { | ||
| op_stride = NPY_MAX_INTP; | ||
| } |
Contributor
There was a problem hiding this comment.
Just for a small contribution to the code review here -- I confirmed that both of the added regression tests fail locally on ARM Mac when I revert this source patch. The full testsuite is passing for me locally on this branch.
The patch does appear to be focused around a real world failure scenario, though it does seem a little obscure and hard to follow.
Probably a more active NumPy core dev should take a look--the patch is relatively small, but the team is getting pretty inundated at the moment.
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
2 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.
PR summary
This PR fixes a critical bug where incorrect coordinates are returned after the iterator buffer boundary for large
(N, 1)index arrays. As seen in real-world applications (e.g., MRI reconstruction inmckib2/pygrappa#107), this can silently produce incorrect results without raising any exceptions.Root Cause:
The issue occurs because the transfer loop is fixed to a zero stride for a size-one inner axis, failing to reflect the updated runtime stride after
remove_multi_indexis applied.Fix:
To resolve this, I modified
numpy/_core/src/multiarray/nditer_constr.cto exclude mutable strides from the fixed-stride optimization.Testing & Verification:
nditerregression tests innumpy/_core/tests/test_nditer.py.unravel_indexregression tests innumpy/lib/tests/test_index_tricks.py.Closes #31980
First time committer introduction
Hi, I am snoopuppy582, a developer using NumPy. I investigated this issue because of its silent but critical impact on real-world calculations, and I hope this patch helps improve the stability of the library.
AI Disclosure
I used AI tools (OpenAI Codex / ChatGPT) strictly for review and verification purposes to ensure the quality of the fix.
I manually analyzed the root cause, wrote the C modifications and regression tests, and executed all local validations. I have fully reviewed and understood the entire diff. Also, this PR description, the commit messages, and all future communications are written entirely by me manually.