MAINT: Cleanup vecdot's signature, typing, and importing
#26313
Conversation
|
(converted to draft since this looks incomplete) |
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.
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.
There was a problem hiding this comment.
So this is an unfortunate limitation of callables that are not just normal python functions: the latter are somewhat unique in that you it is very easy the customize the signature of each individual function at the instance-level; a degree of flexibility that is in pretty much all other cases only possible at the class-level.
Unfortunately (in this context) ufuncs have a bunch of extra methods in attributes, none of which can be adequately typed with normal python functions (as was the case prior to this PR) and thus forcing the use of a single ufunc class (or small set, as we do here with these typing-only nin-/nout-based ufunc classes).
The end result is that, unless you're willing to go through the massive task of creating a single ufunc class with customized signature for every single ufunc, that you're stuck with a less precise signature, one that is unfortunately too imprecise to actually infer the output type of the likes of assert_type(np.vecdot(AR_i8, AR_i8), npt.NDArray[np.signedinteger[Any]]) (a limitation that unfortunately applies to all ufuncs).
| @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.
There was a problem hiding this comment.
I'd say this is a worthwhile change. While to unavoidable regression in dtype-typing support is a shame, it's an unavoidable one and is IMO a worthwhile tradeoff in light of the actual false positives that not bringing in line the vecdot typing with the rest of the ufuncs would bring.
Will be keeping this open for now in case @ngoldbaum has some further comments.
|
Thanks for the context! @mtsokol maybe we could add a release note with a note that because vecdot is now a ufunc, we had to change the type stubs for it to make them less precise because of current limitations around writing types for ufuncs. |
|
Done! |
|
Thanks @mtsokol! |
vecdot's signature, typing, and importingvecdot's signature, typing, and importing
This PR cleans up
vecdottyping, importing, and gufunc's signature. It also specifies array-api-testsvecdotfailure.