-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BUG, MAINT: Improve fromnumeric.py interface for downstream compatibility #7325
Conversation
OOPs, accident :(. Though I am a bit dubious about whether this can't break a lot of subclasses. |
I don't quite understand your comment with the double negative. |
Sorry. Just wondering if all subclasses use the same argument names, and whether it is possible to do C-Api subclasses which might not use compatible parsing at all. Overall, this would be rare, but it would break them, so if it exists, this is probably a no-go. |
I guess all of these are default arguments, so it makes it less likely. Not sure if it is possible or not though. |
Well at least internally, |
Lets move the discussion to the mailing list first. But frankly I am doubtful. |
Sure thing. I am surprised to hear though that you are pessimistic about this being merge-able, as I'm not sure why people wouldn't want to copy the function signature (i.e. use keyword arguments when |
@jreback: Wanted to add you here to the conversation since it was our discussion in the |
Any suggestions on how to proceed on this one? I sent out an email to the |
@shoyer : I ran unit tests for |
I asked @gfyoung to try running the test suites of these external libraries to verify if this change breaks anything... if not, this is a good sign. |
Yeah, sorry for sounding so worried. I have to admit it seems very unlikely anyone does not have identical signatures and using kwargs is always nicer then not. I think I am ok with trying this out and hope nobody finds it irritating. Should maybe have a note in the release notes? |
@seberg : I don't see why not just to be safe! Added. Will ping as soon as tests pass. |
Second breakage reported on the |
Internal method calls to array methods should be written to reflect the actual signature of the argument. Thus, keyword arguments for example should be written explicitly as keyword arguments. This inconsistency has caused libraries like pandas to adopt somewhat awkward function signatures for functions with name equivalents in numpy in which certain arguments had to be externalized for compatibility purposes but would preferably have been hidden in a kwargs argument since they weren't actually used in the implementation. Such externalization would only cause more confusion for the user.
I have somewhat mixed feelings about this. On the one hand, it seems harmless enough -- the idea of IIUC what's really triggering the problem here is that users are insisting on calling Given that we're stuck with calling |
@gfyoung: do you have the time to look through astropy and scipy.sparse for methods like |
In downstream libraries like pandas, they implement methods like 'round' or 'searchsorted' but with slightly different arguments. However, that causes a call like `np.round` to crash because numpy assumes that the object's implementation of the method has a signature that matches that of numpy's.
@njsmith : Yes, you are correct. The issue is that I have in fact checked for backwards compatibility already, as @shoyer made a similar request beforehand. Given that this change will alleviate issues in |
It might be worth a quick manual spot check through the code for
|
Yet another reason why this PR should be merged. If anything, this PR is going to improve downstream compatibility because all signature-incompatibility errors are going to get caught without compromising current compatibility as I have confirmed already. I wouldn't be surprised if other incompatibilities would be found in other downstream libraries if they can be found in a close cousin such as |
+1 on this. and agree fully with @njsmith that |
I would also agree that |
@gfyoung: So the conclusion of your audit of |
@njsmith : I was not able to locate any places where this PR would break compatibility. I only could find places where it would be improved, so there is only gain with merging this, no losses. |
@gfyoung: Okay, thanks very much for your patience and thoroughness in addressing everyone's concerns :-). It sounds like everyone else's concerns have also been addressed, so let's give it a try... |
BUG, MAINT: Improve fromnumeric.py interface for downstream compatibility
@njsmith : Certainly. This compatibility issue has been interesting to work on from all sides of the issue. Thanks for merging this! |
Motivation stemmed from PR in
pandas
in which I was trying to makesearchsorted
inpandas
have a compatible signature withnumpy
without externalizing thesorter
argument (it isn't used in thepandas
implementation) by accepting akwargs
argument to swallow thesorter
arg.However, because
np.searchsorted
treatssorter
like a positional argument in the internal call and NOT as a keyword argument as it should be based on the method signature, it was not possible to do so. This PR fixes that inconsistency as well as in all othernp
functions that use_wrapit
in the implementation.xref #12644 (
pandas
)