API, DEP: Move ufunc signature parsing to the start #18718
Conversation
seberg
force-pushed
the
simplify-typetup
branch
2 times, most recently
from
3374c37
to
c85fcd2
Compare
seberg
added
62 - Python API
| @@ -4,7 +4,7 @@ | |||
| np.sin([1, 2, 3]) | |||
| np.sin(1, out=np.empty(1)) | |||
| np.matmul(np.ones((2, 2, 2)), np.ones((2, 2, 2)), axes=[(0, 1), (0, 1), (0, 1)]) | |||
| np.sin(1, signature="D") | |||
| np.sin(1, signature="D->D") | |||
There was a problem hiding this comment.
@BvB93 I assme that this doesn't need any further changes related to typing, since typing probably doesn't make a difference between the one dtype vs. all dtypes signature versions?
There was a problem hiding this comment.
Naah, it's fine. signature is currently typed as taking either a string or tuple of strings, while this PR only changes which specific strings are allowed, correct?
Line 2930 in 623bc1f
numpy/core/src/umath/ufunc_object.c
Outdated
| PyErr_SetString(PyExc_TypeError, | ||
| "cannot specify both 'sig' and 'signature'"); | ||
| Py_SETREF(*out_typetup, NULL); | ||
| *out_signature = NULL; | ||
| return -1; | ||
| } | ||
| Py_INCREF(sig_obj); |
numpy/core/src/umath/ufunc_object.c
Outdated
| } | ||
| } | ||
| if (nonecount == n) { | ||
| PyErr_SetString(PyExc_ValueError, |
There was a problem hiding this comment.
Since it is unambiguous, maybe just ignore an all-None type tuple? I think we do the same with out.
There was a problem hiding this comment.
Yeah, I agree... The old code deferring parsing to the end had to do this, because by the time it was parsed the normal resolver information would be lost. But if we parse it up-front (which seems clearly better), there is not really a reason not to support it.
| continue; | ||
| } | ||
| if (!PyArray_DescrCheck(item)) { | ||
| /* bad type tuple (maybe not normalized correctly?) */ |
There was a problem hiding this comment.
Shouldn't bad items lead to a warning or exception?
There was a problem hiding this comment.
It drops through to the default resolver, which will raise an error. Tweaked the comment.
This may have slight affects on users, see release notes. Mainly, we have to parse the type tuple up-front (because we need to replace the current type resolution). Since we _must_ de-facto replace the current type resolution, I do not see why we should duplicate code for the odd possibility of someone actually calling `ufunc.type_resolver()` with a `typetup` that is not a actually a type tuple. This commit also deprecates `signature="l"` as meaning (normally) the same as `dtype="l"`.
|
Updated as per meeting yesterday:
|
charris
changed the title
|
Let's get this in for some testing. The coverage defects look like they could be fixed by added tests, but that can be left to a later PR. |
|
Thanks Sebastian. |
|
Thanks for the reviews. Hopefully nobody will notice :). I think most coverage defects are due to the move: The "old code path" can only be reached by calling the resolver directly, which nobody should be doing. (Test can be added though, just has to be done on the C level) |
This may have slight affects on users, see release notes.
Mainly, we have to parse the type tuple up-front (because we need
to replace the current type resolution). Since we must de-facto
replace the current type resolution, I do not see why we should
duplicate code for the odd possibility of someone actually calling
ufunc.type_resolver()with atypetupthat is not a actually atype tuple.
This commit also deprecates
signature="l"as meaning (normally)the same as
dtype="l".