Skip to content

MAINT, TYP: Added missing where typehints in fromnumeric.pyi#20918

Merged
BvB93 merged 4 commits intonumpy:mainfrom
janusheide:add-missing-where-typehints
Jan 28, 2022
Merged

MAINT, TYP: Added missing where typehints in fromnumeric.pyi#20918
BvB93 merged 4 commits intonumpy:mainfrom
janusheide:add-missing-where-typehints

Conversation

@janusheide
Copy link
Copy Markdown
Contributor

Adding typehints for 'where' argument in a couple of functions in fromnumeric, where it appears to be missing.

Copy link
Copy Markdown
Member

@BvB93 BvB93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks pretty good overall but I've got two general comments:

  • The where arguments should be marked as keyword only (matching runtime behavior).
  • Besides the functions in fromnumeric.pyi there are also the identically named methods in the main __init__.pyi file (e.g. ndarray.all). Could you change those as well?

@BvB93 BvB93 added the 09 - Backport-Candidate PRs tagged should be backported label Jan 27, 2022
@BvB93 BvB93 added this to the 1.22.2 release milestone Jan 27, 2022
@janusheide
Copy link
Copy Markdown
Contributor Author

@BvB93 If I understood correctly, your comments should have been addressed now.

I note that there are a bunch of functions in fromnumeric.py, where the where argument is not keyword-only, e.g.

def sum(a, axis=None, dtype=None, out=None, keepdims=np._NoValue,
, I assume this inconsistency is not intentional - and maybe I can use it as an excuse for missing it in the other functions ;)

Wrt. the kw only inconsistency, would you like me to open an issue, a PR or both, or do nothing?

@janusheide janusheide requested a review from BvB93 January 28, 2022 08:16
@BvB93
Copy link
Copy Markdown
Member

BvB93 commented Jan 28, 2022

Wrt. the kw only inconsistency, would you like me to open an issue, a PR or both, or do nothing?

So the move towards the use of keyword-only arguments is somewhat recent, and with this in mind it is no coincidence that the (keyword-only) where arguments addressed in this PR were only added back in numpy 1.20.

Changing all the (much older) remaining where arguments to keyword-only would be technically be backwards compatible. While I doubt they're used much as a positional parameter out in the wild, it's probably not the risk of breaking things.

@BvB93 BvB93 merged commit 62b3482 into numpy:main Jan 28, 2022
@BvB93
Copy link
Copy Markdown
Member

BvB93 commented Jan 28, 2022

Thanks @janusheid

@janusheide
Copy link
Copy Markdown
Contributor Author

Wrt. the kw only inconsistency, would you like me to open an issue, a PR or both, or do nothing?

So the move towards the use of keyword-only arguments is somewhat recent, and with this in mind it is no coincidence that the (keyword-only) where arguments addressed in this PR were only added back in numpy 1.20.

Changing all the (much older) remaining where arguments to keyword-only would be technically be backwards compatible. While I doubt they're used much as a positional parameter out in the wild, it's probably not the risk of breaking things.

I read that as do nothing. I agree that such a change would be backwards incompatible and that one can argue that it is not terribly important. Sometimes it can be helpful to have an issue that others can find if they have the same question, but in this case it it perhaps more of a general problem of kw only args throughout the api.

@InessaPawson
Copy link
Copy Markdown
Member

Hi-five on merging your first pull request to NumPy, @janusheide! We hope you stick around! Your choices aren’t limited to programming – you can review pull requests, help us stay on top of new and old issues, develop educational material, work on our website, add or improve graphic design, create marketing materials, translate website content, write grant proposals, and help with other fundraising initiatives. For more info, check out: https://numpy.org/contribute
Also, consider joining our mailing list. This is a great way to connect with other cool people in our community and be part of important conversations that affect the development of NumPy: https://mail.python.org/mailman/listinfo/numpy-discussion

@charris charris changed the title TYP: Added missing where typehints in fromnumeric.pyi MAINT, TYP: Added missing where typehints in fromnumeric.pyi Jan 29, 2022
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Jan 29, 2022
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.

4 participants