Repository navigation
BUG: make void scalar ellipsis indexing consistent - #32744
Conversation
Signed-off-by: Iason Krommydas <[email protected]>
| if (res == NULL) { | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
just a random check I thought I'd add while I'm at it.
There was a problem hiding this comment.
Unfortunately this change causes a concrete regression in astropy:
import numpy as np
from astropy.uncertainty import Distribution
s = Distribution(np.arange(12.).reshape(3, 4))[0]
x = s[...,]
x.pdf_mean()So, definitely obscure (x = s[...,] is not realistic but maybe someone is doing something like this in a generic function). On numpy main this returns 1.5 but on your PR it's an AttributeError because x is a plain ndarray.
I think the existence of this sort of downstream subclassing might require a FutureWarning? I'm not sure if @mhvk has opinions with an AstroPy hat on.
If it does require a warning cycle, is it really worth the churn to make this change? Sure it improves consistency but we're also not starting NumPy from scratch.
Signed-off-by: Iason Krommydas <[email protected]>
|
Yikes....I would like this from a consistency perspective as it makes void consistent with every other scalar on that aspect but yeah if people are writing such code that would be annoying so I'd want to hear Marten's opinion too. On the other hand, relying somehow on |
|
Let's not worry about astropy in this case. If on main I do the "regular" case, I get an Anyway, I think this PR is a good idea and, given the above, the fact that this PR makes |
It's a typo risk for sure. |
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
LGTM with the nitpick on the release note. Feel free to merge yourself, I think given what Marten and Chuck said this is OK to change without a deprecation.
Signed-off-by: Iason Krommydas <[email protected]>
|
Thanks for the reviews! Let's try this out. In it goes |
|
Thanks Iason |
Signed-off-by: Iason Krommydas <[email protected]>
PR summary
Closes #32742.
voidtype_subscripthad special casing only for the single ellipsis before running every other index throughPyArray_Returnwhich madev[...,]get demoted back tonp.voidwhilev[...]returned a 0d array.We scope
PyArray_Returnto field selection so that every other index behaves as it does on the equivalent 0d array.This felt more natural as other scalar types have no special casing and let
array_subscriptdecide the return type.If you'd rather have another if statement for the 1-element ellipsis tuple, let me know.
Of course this has implications on
np.recordwhich just forwards indexing tonp.voidso the followingreturns an array in both cases on the PR branch. But this is what it should do IMO.
If there is a reason to have the 1-element ellipsis tuple behave differently, we can just close this and the issue.
AI Disclosure
AI wrote the test cause I was lazy