Repository navigation
API [units]: how should we handle forward compatibility against numpy's evolving interfaces ? #15703
Description
Description
This discussion originated in #15691 with @mhvk, and I'll try to give a faithful summary of the problem and the possible solutions here.
We realised that the current approach to implementing function wrappers for numpy NEP18 (array functions) in support of Quantity objects, namely by replicating their signature, sometimes came short as it didn't capture any arguments that happen to be added to numpy functions later on.
An example of this was found in #15691 where np.unique gained a equal_nan argument in version 1.24 (see https://numpy.org/devdocs/reference/generated/numpy.unique.html#numpy-unique). For the sake of simplicity, let's assume that's the only way numpy is ever changing signatures (which even in the wake of numpy 2.0 appears to hold).
My initial thought is that we could provide more robust forward compatibility by making all such wrappers accept catch all *args and **kwargs arguments so that any unknown argument may be passed down to numpy, which by construction retains forward compatibility almost always. The only scenarios I can think of where this is not going to work is if a function gains an out argument, or some other array-like argument that should have some influence over the returned value's unit.
Alternatively, we could stick to the current method and avoid any *args **kwargs indirections by adding a new meta-test to check if signatures are correctly replicated (so we'd catch new arguments in devdeps tests). To me, this approach seems two have a couple downsides:
- it's more work to maintain (and I'm happy to help with it while I'm under contract, but I fear that long term it'll just add to @mhvk's plate)
- users are required to update
astropyin order to use newnumpyinterfaces, which in general is fine but here it's an additional cost that we don't have to pay - it's not 100% clear to me what happens if we just unconditionally add a function argument to a wrapper if that argument doesn't exist in older versions of
numpy. I think it needs to be experimented with.
We could also meet halfway and reserve catch-all arguments for forward compatibility and still add a meta test to check that we capture all known arguments, which may require frequent updates but in most cases wouldn't compel users to upgrade astropy alongside numpy.
Whatever solution is more consensual I'm happy to champion !
Expected behavior
No response
How to Reproduce
No response
Versions
No response
Activity
neutrinoceros commentedon Dec 9, 2023
Actually we have a case study already: 9f2143f (maybe I was a little heavy-handed with the backward compatibility layer, but the bottom line is that it's more work)
mhvk commentedon Dec 9, 2023
In hindsight, I'm not sure we needed to introduce the checks in the wrapper, since for older numpy presumably the function call itself would raise a
TypeError...mhvk commentedon Dec 9, 2023
I think the main argument against adding
*args, **kwargsisTo me, that seems the worst risk - functions can start to give the wrong result due to a change in numpy, which we will not catch until someone alerts us to it.
That said, we do actually use
*args, **kwargsin many helpers (e.g.,invariant_x_helper).A problem that is not mentioned above is what to do if what is added can be positional, but we need some of the argument defaults before, so we cannot use
*args. Inlike_helper, we specifically checkbut one really does not want to start doing that for lots of arguments.
Note that in the existing code,
*args, **kwargsis only used if there are actually further arguments, so in that sense it is different. But it would still be true that if an argument is added that could be aQuantity, we would not notice automatically.neutrinoceros commentedon Dec 9, 2023
I feel this part of the discussion should be conducted in #15691. I'll answer there !
100% agreed here.
You're right. So, my impression is that you're leaning more on the side of preserving the status quo; does the test I propose sound like a good idea to you ? I think it would probably already catch some arguments we haven't ported yet, and it will help protect users from bad surprises when new ones are added.
pllim commentedon Dec 9, 2023
Before we jump into another big refactoring, keep in mind that numpy 2.0 is not something that happens every day. It might be years before numpy 3.0, if there ever will be one. Once numpy 2.0 API is frozen, hopefully such issues would be rare. Not saying that this issue is not valid, but we have bigger fish to fry, e.g.:
neutrinoceros commentedon Dec 9, 2023
Oh but this isn't so much about numpy 2.0 ! really it's about the kind of backward compatible changes that numpy does in seemingly every minor release ! And for what it's worth, I don't think it would be a big refactor to add
*argsand**kwargsto every function wrapper, but it's certainly worth considering against the alternative :)mhvk commentedon Dec 9, 2023
@neutrinoceros - I'm still unsure about the best way forward, but your idea of a test that looks for inconsistent signatures sounds like a good idea regardless of that: I think for now we would just call anything that has
*args, **kwargsas properly matching the signature.neutrinoceros commentedon Dec 9, 2023
Okay, I'll work that test on Monday and I'll probably open a draft PR. Then we can work out how to address the hypothetical failures together. Sounds good to you ?
mhvk commentedon Dec 9, 2023
Excellent!