Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

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

Merged
merged 1 commit into from
May 16, 2025
Merged

Conversation

lvllvl
Copy link
Contributor

@lvllvl lvllvl commented May 5, 2025

Attempts to close #28641

@jorenham jorenham self-requested a review May 5, 2025 18:28

This comment has been minimized.

Copy link
Member

@jorenham jorenham left a 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?

numpy/typing/tests/gh28641_argmin_argmax.py Outdated Show resolved Hide resolved
numpy/_core/fromnumeric.pyi Outdated Show resolved Hide resolved

This comment has been minimized.

This comment has been minimized.

@jorenham
Copy link
Member

In case it wasn't clear yet: Fixing #28641 requires narrowing the out typevar bound of np.arg{min,max} (and np.ndarray.arg{min,max}) from bound=NDArray[Any] to bound=NDArray[np.bool | np.integer]. So there's no need to touch the overloads :)

@lvllvl lvllvl force-pushed the issue-28641 branch 4 times, most recently from d1ffd4f to 91ea834 Compare May 14, 2025 21:36
@lvllvl lvllvl force-pushed the issue-28641 branch 9 times, most recently from e536b84 to 600499b Compare May 15, 2025 18:17
@lvllvl
Copy link
Contributor Author

lvllvl commented May 15, 2025

In case it wasn't clear yet: Fixing #28641 requires narrowing the out typevar bound of np.arg{min,max} (and np.ndarray.arg{min,max}) from bound=NDArray[Any] to bound=NDArray[np.bool | np.integer]. So there's no need to touch the overloads :)

Thanks for your help, I adjusted tests that started to fail after I changed the stubs.

Do you think additional testing is needed?

@lvllvl lvllvl marked this pull request as ready for review May 15, 2025 18:41
@lvllvl lvllvl requested a review from jorenham May 15, 2025 21:06
@jorenham
Copy link
Member

Do you think additional testing is needed?

It wouldn't hurt to check that this now indeed rejects non-integer (or bool) arrays for out in typing/tests/data/fail

Copy link
Member

@jorenham jorenham left a 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.

@lvllvl lvllvl force-pushed the issue-28641 branch 2 times, most recently from 6842d13 to 88cf0c8 Compare May 16, 2025 00:42
numpy/__init__.pyi Outdated Show resolved Hide resolved
@lvllvl lvllvl force-pushed the issue-28641 branch 6 times, most recently from 22f4a1b to 5932c98 Compare May 16, 2025 17:29
@jorenham
Copy link
Member

I'd prefer it if you would run the test locally: CI isn't free, and each push notifies me :)

@lvllvl
Copy link
Contributor Author

lvllvl commented May 16, 2025

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!

@jorenham jorenham merged commit 7beea21 into numpy:main May 16, 2025
75 of 76 checks passed
@jorenham
Copy link
Member

thanks @lvllvl :)

@lvllvl
Copy link
Contributor Author

lvllvl commented May 16, 2025

thanks @lvllvl :)

Thanks for your help, I learned a lot! V fun.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TYP: np.argmin overloads could be more precise
2 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.