Repository navigation
Quantity.__class_getitem__ cannot use super on ndarray.__class_getitem__ on python >=3.10 #12510
Description
All PRs are currently failing their python 3.10 + devdeps tests because the interaction between Quantity.__class_getitem__ and ndarray.__class_getitem__ is not correct. E.g.,
In [2]: u.Quantity[u.m]
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
<ipython-input-2-f0c088963c1a> in <module>
----> 1 u.Quantity[u.m]
~/data/mhvk/packages/astropy/astropy/units/quantity.py in __class_getitem__(cls, unit_shape_dtype)
404 # metadata using Annotated.
405 if not NUMPY_LT_1_22:
--> 406 cls = super().__class_getitem__((cls, *shape_dtype))
407 return Annotated.__class_getitem__((cls, unit))
408
TypeError: Too few arguments for Quantity
It seems that ndarray.__class_getitem__ cannot be called without an actual (shape, dtype). Those also should be meaningful, meaning that the current test u.Quantity[u.m, "more"] would seem to incorrectly associate this "more" with the ndarray aspect.
In [13]: u.Quantity[u.m, "more"]
Out[13]: typing.Annotated[astropy.units.quantity.Quantity[astropy.units.quantity.Quantity, 'more'], Unit("m")]
My sense is to remove the super() call... @nstarman??
Activity
mhvk commentedon Nov 21, 2021
See #12511 for a PR that implements my suggestion of just not using
NDArray.__class_getitem__for now.nstarman commentedon Nov 22, 2021
Sounds good to me. We can re-add it with some tests on python 3.10. and numpy 1.22+.
I also think the
u.Quantity[u.m, "more"]should be changed to fail regardless. As in we should always have the arguments be (unit, shape, dtype) regardless of whether the latter two are functional.pllim commentedon Dec 6, 2022
Is this resolved?
mhvk commentedon Dec 6, 2022
I thought the fix I put in was meant to only be temporary. Should
Quantity.__class_getitem__not be compatible withnumpy.__class_getitem__?pllim commentedon Dec 6, 2022
Ah, okay. Maybe the title needs to be updated. "numpy dev" as of Nov 2021 would have been stable release by now.
[-]Tests failing with numpy-dev due to Typing[/-][+]Quantity.__class_getitem__ cannot use super on ndarray.__class_getitem__ on python >=3.10[/+]mhvk commentedon Dec 6, 2022
Good point. I changed the title.
mhvk commentedon Dec 6, 2022
Also removed
numpy-dev, but perhaps this is a case forupstream-fix-required? @nstarman?mhvk commentedon Apr 7, 2023
@nstarman - seeing your typing updates in #14286, just a reminder that perhaps this issue can be closed with the subsequent update to the minimum version of numpy (even if we just add something that only works for
not NUMPY_LT_1_22).neutrinoceros commentedon Feb 21, 2024
@mhvk that would be #15706 ?
so, should we just close this one ?
mhvk commentedon Feb 21, 2024
Let's ping @nstarman about this - right now, we are not calling
ndarray.__class_getitem__ever, which means we cannot indicate a shape or dtype in the typing. Should we change that?github-actions commentedon Feb 22, 2024
Hi humans 👋 - this issue was labeled as Close? approximately 15 hours ago. If you think this issue should not be closed, a maintainer should remove the Close? label - otherwise, I will close this issue in 7 days.
If you believe I commented on this issue incorrectly, please report this here
nstarman commentedon Feb 22, 2024
Yes. This kept getting pushed from my attention because I think I made a mistake in setting up the type hint. IMO what we should do (explained https://github.com/nstarman/astropy-APEs/blob/units-quantity-2.0/APE25/report.pdf) is make it
Quantity[ValueType, UnitStuff], so one would doQuantity[NDArray[float], "length"].For backwards compatibility we should override
__class_getitem__and implement aQuantityU(or similarly named) GenericAlias that is parametrized only by the unit stuff — akin to hownumpy.typing.NDArray[dtype]drops the shape stuff fromnumpy.ndarray[shape, dtype].mhvk commentedon Feb 23, 2024
That all makes sense. So, just to be sure, one would still be able to do
Quantity["length"], right? I.e., no import ofQuantityUneeded -- that would just be used internally?github-actions commentedon Feb 29, 2024
I'm going to close this issue as per my previous message, but if you feel that this issue should stay open, then feel free to re-open and remove the Close? label.
If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here