ENH: NEP 50 scalar promotion semantics in ufunc.at - #32094
Conversation
| * TODO: Just like the general dual NEP 50/legacy promotion | ||
| * support this is meant as a temporary hack for NumPy 1.25. |
There was a problem hiding this comment.
@seberg what do you think about this "temporary hack"?
There was a problem hiding this comment.
FWIW, this seems fine to do (just like the other PR was, of course not back-porting).
The "temporary hack" would require removing legacy promotion entirely, which in part would be nice, but in it's entirety seems hard...
(I think I once thought that if we stop using it in NumPy, one could at least move the hack to a different place for user ufunc and slowly deprecate)
mhvk
left a comment
There was a problem hiding this comment.
@ngoldbaum - looks very good! And nice that it is relatively simple. Only some small comments.
| Py_INCREF(tmp_operands[1]); | ||
| operand_DTypes[1] = NPY_DTYPE(PyArray_DESCR(op2_array)); | ||
| Py_INCREF(operand_DTypes[1]); | ||
| if (mark_pyscalar_operand( |
There was a problem hiding this comment.
Could in principle directly assign to op2_is_pyscalar, but arguably what you have is clearer, so only writing this in case you feel differently.
ikrommyd
left a comment
There was a problem hiding this comment.
I honestly do not see anything here. All looks good to me. The only OCD thing that the other PR that I did #32090 was labeled as a BUG while this is an ENH. I say they should be the same so this one needs changing or the already merged on. The release notes are labeled in both PRs as "change". Only left a minor nitpicky comment for the goto label.
| /* Find the correct operation_descrs for the operation */ | ||
| int resolve_result = resolve_descriptors(nop, ufunc, ufuncimpl, | ||
| tmp_operands, operation_descrs, signature, operand_DTypes, NULL, NPY_UNSAFE_CASTING); | ||
| finish_resolution: |
There was a problem hiding this comment.
nit: this should probably not be indented in. fail: is usually at column 0
IMO the other one should have been labeled as ENH because it changes behavior. No need to go back and correct that though. |
mhvk
left a comment
There was a problem hiding this comment.
Looks all OK to me, thanks!
PR summary
Followup for #32076 (comment)
Also see #32090.
AI Disclosure
I used AI to identify the issue and help debug the fix.