BUG: resolve incompatibilities in units and utils.masked with numpy 2.4 (dev)#18821
BUG: resolve incompatibilities in units and utils.masked with numpy 2.4 (dev)#18821mhvk merged 4 commits intoastropy:mainfrom
units and utils.masked with numpy 2.4 (dev)#18821Conversation
|
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.
|
units and utils.masked with numpy 2.4 (dev)
f8ebdbc to
1f8c5c9
Compare
| # also work around this by passing on a formatter (as is done in Angle). | ||
| # So, we do nothing if the formatter argument is present and has the | ||
| # relevant formatter for our dtype. | ||
| formatter = args[6] if len(args) >= 7 else kwargs.get("formatter") |
There was a problem hiding this comment.
this was a subtle one. The fact that argument positions where hard coded made it hard to find. Instead of branching explicitly on numpy's version, I rewrote this to dynamically compute the position at runtime. However, I'm intentionally leaving import inspect as a "lazy" import because importing this module is noticeable in the context of a CLI's starting time.
There was a problem hiding this comment.
Do you mean that formatter is no longer the sixth positional argument? That seems really weird (and a bug!) Or am I missing something?
There was a problem hiding this comment.
That is exactly what I mean, and why positional-allowed arguments are basically impossible to deprecate/remove without breaking someone else's code.
| import inspect | ||
|
|
||
| sig = inspect.signature(np.array2string) | ||
| formatter_arg_pos = list(sig.parameters).index("formatter") - 1 |
There was a problem hiding this comment.
note to reviewers (which I assume would be @mhvk): I don't understand why this -1 is needed, but empirically, that's what works.
|
I found the remaining issue (this was significantly harder to find). I also included a third commit, which is a purely internal refactor and doesn't fix any known issue per-se, but should reduce the "attack surface" for the kind of incompatibilities I'm solving here, so hopefully it makes sense to include it here. |
| args = (self.ma, None, None, None, ", ", "", np._NoValue, {"int": hex}) | ||
| else: | ||
| args = (self.ma, None, None, None, ", ", "", {"int": hex}) | ||
| out3 = np.array2string(*args) |
There was a problem hiding this comment.
Maybe this part of the test should actually be removed: it seems redundant with in depth signature introspection tests, and it clearly has a maintenance cost of its own. Anyway, that's merely a suggestion for a follow up, but I think it's out of scope here.
|
What about these 2 failures? |
|
That's expected. As often, I'm ahead of nightlies, so it will not pass allowed-failure until Wednesday, by which point it will be required |
mhvk
left a comment
There was a problem hiding this comment.
I think this looks good but I wonder if even independently of numpy we should just insist formatter is given as a keyword argument... I don't like inspecting the call signature to find the position - really, positions should not be allowed to change.
| # also work around this by passing on a formatter (as is done in Angle). | ||
| # So, we do nothing if the formatter argument is present and has the | ||
| # relevant formatter for our dtype. | ||
| formatter = args[6] if len(args) >= 7 else kwargs.get("formatter") |
There was a problem hiding this comment.
Do you mean that formatter is no longer the sixth positional argument? That seems really weird (and a bug!) Or am I missing something?
| if NUMPY_LT_2_4: | ||
| args = (self.q, None, None, None, ", ", "", np._NoValue, {"float": str}) | ||
| else: | ||
| args = (self.q, None, None, None, ", ", "", {"float": str}) |
There was a problem hiding this comment.
I guess it is true, though - I think I should raise an issue over at numpy, they should just not make it positional any more... numpy/numpy#30068 (comment)
Your PR doesn't appear to meet controversy, so I may just wait for it to land before I mirror that move here. Thanks for taking it back to numpy ! |
…y with numpy 2.4 BUG: fme
… (dev). The test still fails on a different failure mode though
…to reduce the risk of a mistake
1f8c5c9 to
ef1c8e0
Compare
c81274f to
590b283
Compare
|
This now also fixes all newly detected signature mismatches discovered thanks to numpy/numpy#30143, and which showed as 26 new failures in unstable CI. |
|
and there seems to be more incoming (numpy/numpy#30147) so I'd suggest merging this one promptly in order to keep unstable CI's logs manageable and useful. |
mhvk
left a comment
There was a problem hiding this comment.
Agreed, let's just put this in, so the tests work again. I think if my numpy PR is merged, we will be alerted by a failure anyway!
Thanks!!
…and `utils.masked` with numpy 2.4 (dev)
…821-on-v7.2.x Backport PR #18821 on branch v7.2.x (BUG: resolve incompatibilities in `units` and `utils.masked` with numpy 2.4 (dev))
Description
xref: numpy/numpy#30068
xref: numpy/numpy#30143
At the time of opening, the test in
unitsthat I'm patching still fails against numpy-dev, where the current failure mode bisects to the same PR (specifically, 41cebd0f891f54d3090d4544d3f7a932ec0d4782, which is the first commit there), and may indicate an actual regression. Though, I suspect the remaining problem is more likely to be on our side.I haven't figured it out yet, but here's the error I'm getting:
These incompatibilities are expected to show up in CI with numpy's next nightly, ~2 days from now.
I'm hoping to figure out the remaining problem ahead of that.