API, DEP: Move ufunc signature parsing to the start#18718
API, DEP: Move ufunc signature parsing to the start#18718charris merged 4 commits intonumpy:mainfrom
Conversation
3374c37 to
c85fcd2
Compare
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
mhvk
left a comment
There was a problem hiding this comment.
Since this touched the ufuncs, I thought I would have look, but didn't really get to any depth. Still, the few comments may be useful. Overall, it certainly makes sense!
numpy/core/src/umath/ufunc_object.c
Outdated
numpy/core/src/umath/ufunc_object.c
Outdated
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.
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:
|
|
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".