BUG: Any dtype should call square on arr ** 2#29392
Conversation
… array type except object arrays
MaanasArora
force-pushed
the
bug/structured-arr-fast-square
branch
from
81f9adf to
d8eeb76
Compare
MaanasArora
changed the title
square on arr ** 2square on arr ** 2
|
|
||
| PyArrayObject *a1 = (PyArrayObject *)o1; | ||
| if (!(PyArray_ISFLOAT(a1) || PyArray_ISCOMPLEX(a1))) { | ||
| if (PyArray_ISOBJECT(a1)) { |
There was a problem hiding this comment.
Thanks, yeah this seems right/safe to me. I am not sure I see why **2 confuses a custom __array_ufunc__, but I don't think this is an issue (and probably restores old behavior). (If we had a bigger reason to keep it as is, I would be tempted to ask downstream to fix it there.)
Curious, does **0.5 always end up with sqrt()?
(Maybe the nicer solution for at least **2 would actually be to make the object square implementation use **2 internally then this special case wouldn't even be needed. But no priority so if, let's do that as a follow-up.)
seberg
left a comment
seberg
left a comment
There was a problem hiding this comment.
Anyway, looks right, will put it in a bit. Thanks a lot!
| if (PyArray_ISOBJECT(a1)) { | ||
| return 1; | ||
| } | ||
| if (!is_square && !PyArray_ISFLOAT(a1) && !PyArray_ISCOMPLEX(a1)) { |
There was a problem hiding this comment.
Actually, sorry should have looked closer. This is either square or sqrt. Maybe we can just remove this branch also for sqrt?
Or would that change behavior drastically from the original version? (I think I would lean towards that.
(If not, it may be nice to just use fastop != nops.square rather than introducing a new variable, but nitpicking.)
There was a problem hiding this comment.
Thanks for reviewing! It seems that in the old behavior only square was special-cased, as far as I can tell:
It's a bit confusing, so would have been nice to have gotten rid of it, but well :)
(If not, it may be nice to just use fastop != nops.square rather than introducing a new variable, but nitpicking.)
Actually yes, let me remove the variable, thanks!
There was a problem hiding this comment.
Then maybe let's keep it at that for this PR... So we can consider backporting it (even if I am not sure I consider it a real regression).
…ast_scalar_power` function
|
Thanks @MaanasArora, let's put this in. This should be simple enough to backport, it is a regression, but also a rather niche detail that I think is shaky to rely on. |
* BUG: update fast_scalar_power to handle special-case squaring for any array type except object arrays * BUG: fix missing declaration * TST: add test to ensure `arr**2` calls square for structured dtypes * STY: remove whitespace * BUG: replace new variable `is_square` with direct op comparison in `fast_scalar_power` function
BUG: Any dtype should call `square` on `arr ** 2` (#29392)
* BUG: update fast_scalar_power to handle special-case squaring for any array type except object arrays * BUG: fix missing declaration * TST: add test to ensure `arr**2` calls square for structured dtypes * STY: remove whitespace * BUG: replace new variable `is_square` with direct op comparison in `fast_scalar_power` function
3 participants
Fixes #29388.
The simplification in gh-27901 seems to have removed special casing for squares, not considering the differing behavior of
np.power(x, 2)andnp.square(x)on structured dtypes.There may be more edge cases to consider--I am looking! Also adding a test. Thanks.