BUG: fix forward compatibility for np.arange(<Quantity>, ...) against numpy 2.4 (dev)#18849
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
Welcome to Astropy 👋 and congratulations on your first pull request! 🎉 A project member will respond to you as soon as possible; in the meantime, please have a look over the Checklist for Contributed Code and make sure you've addressed as many of the questions there as possible. If you feel that this pull request has not been responded to in a timely manner, please send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail feedback@astropy.org. |
|
👋 Thank you for your draft pull request! Do you know that you can use |
b143697 to
8bc9f1f
Compare
np.arange(<Quantity>, ...)\ against numpy 2.4 (dev)np.arange(<Quantity>, ...) against numpy 2.4 (dev)
6eb9f2e to
53e0d4a
Compare
| # reverse positional arguments so `stop`` always comes first | ||
| # this is done to ensure that the arrays are first converted to the | ||
| # expected unit, which we guarantee should be stop's | ||
| args_rev, out_unit_loc = _quantities2arrays(*reversed(qty_args)) |
There was a problem hiding this comment.
overall, I'm not sure using _quantities2arrays is a good idea here, but it's the easiest way to ensure backward compat with the previous implementation, so I'll give it a rest for now.
d5084e1 to
87a3d95
Compare
|
This might take a little while to get properly reviewed, for understandable reasons, but since it works out of the box even now (while the breaking change in numpy hasn't landed in a nightly yet), let's open it for review to maximize visibility ! |
87a3d95 to
906e7a3
Compare
|
Hum actually, I'm think the upstream PR I'm mirroring here should be taken with a grain of salt: for instance, the |
|
oh actually, this is explicitly refered to in the changelog entry associated to that PR, I just missed it so, it is actually intended that type checkers now reject |
First things that come to mind are usage signature, call/call-time signature, access/acces-time signature, true/actual signature, hidden/latent signature, supported signature, or "dark
I suppose it would be fair to blame numpy or me for this, if it turns out to be needed
Yea that's not unlikely. It might even happen accidentally, since tools like stubtest wouldn't even notice it, and I'm not sure if we have unit-tests for it. There's also secret option no. 3: change the runtime ( arange.__signature__ = inspect.signature(np.arange) # or construct it from scratchor slightly more hacky: arange.__text_signature__ = np.arange.__text_signature__ # or just copy-paste itThe first one is the least hacky, but still kinda hacky. It's nowhere near as hacky as what numpy does though 😅 |
…st numpy 2.4 (dev)
6cb61a2 to
564d9e7
Compare
I'll keep this in mind 😈 (seriously though, I'll take a bet that no-one is actively relying on this)
great, so I think everyone is on board with option 1 then !
I don't want to know unless I have to 🙈 |
|
Hopefully this is ready for a final review now. I should have finally answered all comments, but of course do feel free to remind me if I accidentally missed some ! |
mhvk
left a comment
There was a problem hiding this comment.
It still feels like overly long and thus distracting amount of code, but given that it is a corner case, I don't want to belabor this too much further. However, two in-line comments still about replacing reversed with [::-1], which really is fine if one knows one has a tuple (as we do here).
| # reverse positional arguments so `stop` always comes first | ||
| # this is done to ensure that the arrays are first converted to the | ||
| # expected unit, which we guarantee should be stop's | ||
| args_rev, out_unit = _quantities2arrays(*reversed(qty_args)) |
There was a problem hiding this comment.
I hadn't seen this before, but you have just constructed qty_args, so you know it is a tuple -- no reason not to do qty_args[::-1].
Or even construct them reversed; indeed, the whole match and this part can be combined by doing,
if start is None:
q_stop = _as_quantity(stop)
args_rev, out_unit = (q_stop.value,), q_stop.unit
else:
args_rev, out_unit = _quantities2arrays(stop, start)
There was a problem hiding this comment.
I don't think combining operations that can be separate makes the code any simpler.
There was a problem hiding this comment.
Fine to now do the full think, but please do just use _quantities2arrays(*qty_args[::-1]) (probably removes need to even import reversed.
Just to state the logic for my other suggestion: It makes it shorter, which helps for the clarity of the module as a whole. For a function like this, I'd rather have a single implementation that is dense and well-commented, to make going through the file easier for all the other functions. Anyway, I know that this is very much a matter of taste, so no worries!
There was a problem hiding this comment.
coming right up.
For full disclosure, and because I fear I might be sounding a bit blunt; I'm at the end of a tiresome day where I've been juggling with at least 3 concurrent issues in devdeps CI, including this one. My priority is to get it back to green so it becomes easy again to understand logs. Anyway, apologies for my tone in previous responses, I'm trying my best but it's not always enough.
There was a problem hiding this comment.
@neutrinoceros - no worries, my dutch self had not even noticed any bad tone or anything... (and I'm a bit in the same boat in the sense of currently having a bit too much on my plate). Anyway, FWIW, I find our discussions usually end up with things in a good place!
There was a problem hiding this comment.
Me too ! Good luck with your plate 😅
| assert out_unit == stop.unit | ||
|
|
||
| # reverse args again to restore initial order | ||
| args = tuple(reversed(args_rev)) |
There was a problem hiding this comment.
I'm still confused: args_rev is guaranteed to be a tuple (since quantities2arrays returns a tuple). So the idiomatic way would seem to be args = args[::-1].
|
@mhvk if you're satisfied with the current state of things, please use a squash-merge for this one. Thank you ! |
mhvk
left a comment
There was a problem hiding this comment.
OK, let's get it in. Will be nice to have devdeps green again!
|
@meeseeksdev backport to v7.2.x |
…nge(<Quantity>, ...)` against numpy 2.4 (dev)
…nge(<Quantity>, ...)` against numpy 2.4 (dev)
Description
xref: numpy/numpy#30147
This one was unusually hard to pull of, because
np.arange, mimicking the builtinrange, has a very peculiar signature, where the meaning of the first positional argument depends on the rest of the arguments. A full refactor of the wrapper function ended up being necessary to make this forward compatible. Hopefully inline comments should help understanding why.Because it's taken so long to wrap things up (I've been playing whac-a-mole on-and-off on this for 2 days), I haven't yet collapsed my commits. I'd like to take the current state for a spin on CI first.