MAINT: Cleanup vecdot's signature, typing, and importing
#26313
Conversation
|
(converted to draft since this looks incomplete) |
mtsokol
force-pushed
the
vecdot-signature
branch
3 times, most recently
from
a2d2d5f
to
37568cf
Compare
| 'full', 'full_like', 'matmul', 'vecdot', 'shares_memory', | ||
| 'may_share_memory', '_get_promotion_state', '_set_promotion_state'] |
There was a problem hiding this comment.
I wanted vecdot to be closer to matmul in imports/__all__ as they are both gufuncs.
mtsokol
changed the title
vecdot signaturevecdot's signature, typing, and importing
|
Hi @ngoldbaum, I think I finally figured out why On my macos which skips array-api signature tests. Therefore I couldn't reproduce it locally. But when running it on a linux machine, it turns out that for all ufuncs and CI job array-api-tests suite runs on It turns out that the array-api-test signature test pays more attention to keyword arguments rather than positional ones. This caused If I figured it out correctly then the test skip must stay (I updated the note explaining why it's skipped) until ufuncs have proper signatures for CC @mhvk I updated typing stubs for |
|
Sounds like some nice sleuthing was involved here. I'm afraid I don't understand the typing sufficiently well to comment further, though! |
| assert_type(np.vecdot.nargs, Literal[3]) | ||
| assert_type(np.vecdot.signature, Literal["(n),(n)->()"]) | ||
| assert_type(np.vecdot.identity, None) | ||
| assert_type(np.vecdot(AR_f8, AR_f8), Any) |
There was a problem hiding this comment.
There are a bunch of other tests like this that got deleted. Why only this one now? Or just an oversight?
There was a problem hiding this comment.
It would improve my confidence that the typing changes are correct if all the tests that used to be in reveal/numeric.pyi were reproduced here.
There was a problem hiding this comment.
Tests from reveal/numeric.pyi were associated with the removed typing stubs (numpy/_core/numeric.pyi) that I added when vecdot was implemented in pure Python. Later, when vecdot was reimplemented as a gufunc the stubs should have been removed.
As of now vecdot typing stub is coming from _GUFunc_Nin2_Nout1 only, and this: assert_type(np.vecdot(AR_f8, AR_f8), Any) is the only test that is valid with the current typing (same for matmul).
I wanted to mimic matmul implementation, as they are both gufuncs.
If we want to keep removed tests then there's a question how we want to proceed: Remove GUFunc based typing stub and restore numpy/_core/numeric.pyi typing stubs (but then the same refactoring could apply to matmul and others) or do we want to stick to GUFunc typing stub.
There was a problem hiding this comment.
assert_type(np.vecdot(AR_f8, AR_f8), Any) is the only test that is valid with the current typing
Can you explain a little more why e.g. assert_type(np.vecdot(AR_i8, AR_i8), npt.NDArray[np.signedinteger[Any]]) isn't valid? Is this a limitation of the automatically generated typing for gufuncs?
If so, it sounds like the hand-written type overrides for vecdot should be used and similar overrides should be written for matmul, or the automatically generated typing should be improved to capture the semantics of the old hand-written types. Users might write types using those in numpy 2.0 (unless you're looking to backport this to 2.0, which might not be possible given timing) so we shouldn't make the types less expressive or possibly break typing tests by changing semantics.
I'll defer to @BvB93 or others who are more experienced dealing with types about all this though.
There was a problem hiding this comment.
IIUC it's a limitation of GUFunc typing stub. For me we can keep vecdot overrides as before.
| @overload | ||
| def vecdot( | ||
| x1: _ArrayLikeObject_co, x2: _ArrayLikeObject_co, axis: int = ... | ||
| ) -> NDArray[object_]: ... |
There was a problem hiding this comment.
Why can these type annotations be deleted?
There was a problem hiding this comment.
Ah because vecdot is a ufunc now and ufunc.pyi handles it for us, right?
There was a problem hiding this comment.
Yes! For example there is no def matmul(...) in pyi files.
| # TODO: check why in CI `inspect.signature(np.vecdot)` returns (*arg, **kwarg) | ||
| # instead of raising ValueError. mtsokol: couldn't reproduce locally | ||
| # ufuncs signature on linux is always <Signature (*args, **kwargs)> | ||
| # np.vecdot is the only ufunc with a keyword argument which causes a failure |
There was a problem hiding this comment.
Do the array API tests also check that vecdot's signature is correct using duck typing? Probably worth doing both - check the signature and validate that the signature is correct!
There was a problem hiding this comment.
There is test_signatures.py in array-api-tests suite for testing signatures. Separately, functions are tested by calling them with args+kwargs, like test_vecdot in test_linalg.py.
This PR cleans up
vecdottyping, importing, and gufunc's signature. It also specifies array-api-testsvecdotfailure.