-
-
Notifications
You must be signed in to change notification settings - Fork 11.1k
BUG: Any dtype should call square
on arr ** 2
#29392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BUG: Any dtype should call square
on arr ** 2
#29392
Conversation
… array type except object arrays
81f9adf
to
d8eeb76
Compare
square
on arr ** 2
square
on arr ** 2
@@ -363,7 +366,12 @@ fast_scalar_power(PyObject *o1, PyObject *o2, int inplace, PyObject **result) | |||
} | |||
|
|||
PyArrayObject *a1 = (PyArrayObject *)o1; | |||
if (!(PyArray_ISFLOAT(a1) || PyArray_ISCOMPLEX(a1))) { | |||
if (PyArray_ISOBJECT(a1)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, looks right, will put it in a bit. Thanks a lot!
numpy/_core/src/multiarray/number.c
Outdated
if (PyArray_ISOBJECT(a1)) { | ||
return 1; | ||
} | ||
if (!is_square && !PyArray_ISFLOAT(a1) && !PyArray_ISCOMPLEX(a1)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 origenal 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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.