-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
TYP: np.argmin and np.argmax overload changes #28906
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
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain why you chose to remove some of the overloads, and why you modified the parameter types and return types of the other ones?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
In case it wasn't clear yet: Fixing #28641 requires narrowing the |
d1ffd4f
to
91ea834
Compare
e536b84
to
600499b
Compare
Thanks for your help, I adjusted tests that started to fail after I changed the stubs. Do you think additional testing is needed? |
It wouldn't hurt to check that this now indeed rejects non-integer (or bool) arrays for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The np.arg{min,max}
functions look good now 👌🏻.
Since they're a a bit of a package deal with the np.ndarray.arg{min,max}
methods, it would be best to also change those in the same way.
6842d13
to
88cf0c8
Compare
22f4a1b
to
5932c98
Compare
I'd prefer it if you would run the test locally: CI isn't free, and each push notifies me :) |
oh my mistake, ok will do! |
thanks @lvllvl :) |
Thanks for your help, I learned a lot! V fun. |
Attempts to close #28641