Skip to content
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

ENH: add annotation for abs #16618

Merged
merged 3 commits into from
Jun 21, 2020
Merged

ENH: add annotation for abs #16618

merged 3 commits into from
Jun 21, 2020

Conversation

unnonouno
Copy link
Contributor

numpy.abs is alias of numpy.absolute. I added its type annotation.

@@ -737,6 +737,7 @@ class ufunc:
@property
def at(self) -> Any: ...

abs: ufunc
Copy link
Member

Choose a reason for hiding this comment

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

Considering it's an alias, why not just define it as such?

Suggested change
abs: ufunc
abs = absolute

Copy link
Member

Choose a reason for hiding this comment

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

Also if we could add a really simple test case to numpy/tests/typing/pass/ufuncs.py that would be great!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for reviewing. Ok, I'll add a test case. BTW, I found that ufuncs.py contain no other ufuncs except sin. Is it Ok to add a test case only for abs, or is it better to add all other ufuncs? (or parameterized test?)

Copy link
Member

Choose a reason for hiding this comment

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

Is it Ok to add a test case only for abs, or is it better to add all other ufuncs? (or parameterized test?)

I feel it's fine as it is now, especially since the (currently) rather broad ufunc.__call__() signature will make adding proper fail tests somewhat tricky.
Still, if you're interested feel free to add them, be it either in this pull request or another one.

@seberg
Copy link
Member

seberg commented Jun 16, 2020

Would it be possible to have tests which check that the typing module is a subset of the public namespace? Or is that unnecessary? We may occasionally remove things from the public API and it would be nice to notice it automatically. E.g. I just merged a try to deprecate np.float, etc. which alias the Python types.

@BvB93
Copy link
Member

BvB93 commented Jun 16, 2020

Would it be possible to have tests which check that the typing module is a subset of the public namespace?

That would be rather tricky, as far as I'm aware there is no straightforward way of importing content directly from a .pyi file.

Besides, if something is removed then one of the typing-related tests is likely to break anyway, especially since a number of them are also executed at run-time for this very reason.

@person142
Copy link
Member

Would it be possible to have tests which check that the typing module is a subset of the public namespace?

We had something that did the reverse of that (found things in NumPy not in the stubs) in the stubs repo:

https://github.com/numpy/numpy-stubs/blob/master/runtests.py#L94

(It finds things in the stubs by walking the ast.) Something like that could be adapted into a test (though Python ast is not stable, so that's something to think about).

Regardless of tests, I should add that back over here; it's how I kept issues like #16546 up to date.

@person142 person142 merged commit e481335 into numpy:master Jun 21, 2020
@person142
Copy link
Member

CI failure unrelated, so merging. Thanks @unnonouno!

@unnonouno
Copy link
Contributor Author

Thank you!

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.

None yet

4 participants