Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork10.9k
ENH: Use array indexing preparation routines for flatiter objects#28590
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
198df6b to75aaed0Compare75aaed0 to9f2d51fCompare| assert_raises(ValueError, ia, x.flat, s, np.zeros(9, dtype=float)) | ||
| assert_raises(ValueError, ia, x.flat, s, np.zeros(11, dtype=float)) | ||
| assert_raises(IndexError, ia, x.flat, s, np.zeros(9, dtype=float)) | ||
| assert_raises(IndexError, ia, x.flat, s, np.zeros(11, dtype=float)) |
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.
While this is certainly more consistent and I'd even call it a bugfix, it is a behavior change and someone might have code relying on the old behavior. Needs a release note at least. You also need another release note for the new features.
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.
Agreed. Added a release note that lists all the most important changes.
8f2b322 to33109ddCompareThis is a big refactor, so I think we'll need at least two experienced developers to go over the C code changes, so that might take a while. I'll try to do a pass focusing on the correctness of the C code soon. On a first, high-level pass this looks like mostly simplification and cleanup. I think you should also try running the indexing benchmarks to see if there are any significant regressions in existing benchmarks. I think It would also be nice to get new entries in the |
lysnikolaou commentedMar 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
These are the results of running the (old & new) benchmarks: It looks like having special cases for tuple, ellipses etc. (instead of going through |
Probably |
I added a couple of special cases for an empty tuple and boolean indexes. This fixes the two worst performance regressions. I feel that the rest are acceptable, since this goes through a much more complex code path to make sure that everything is set up correctly. |
I added the 2.3 milestone to make sure we don't drop reviewing this before cutting the release. |
@ngoldbaum I am about to push this off to 2.4 unless you want to put it in very soon. |
I spoke with Lysandros and he said it's OK to push this off. We'll coordinate on getting this reviewed soon. |
prepare_indexiniter_subscriptanditer_ass_subscript. This fixes various cases that were broken before:arr.flat[[True, True]]arr.flat[[1.0, 1.0]]arr.flat[()] = 0flatiterindexing operationsCloses#28314.