BUG: Do not elide complex abs() #9110
Conversation
complex abs() results in float so it cannot be elided. Closes numpygh-9109
|
as far as I can tell abs() the only special unary ufunc that changes types |
numpy/core/tests/test_multiarray.py
Outdated
| @@ -2850,6 +2850,9 @@ def test_temporary_with_cast(self): | |||
| d = f.astype(np.float64) | |||
| assert_equal(((f + f) + d).dtype, np.dtype('f8')) | |||
|
|
|||
| c = np.ones(100000, dtype=np.complex) | |||
There was a problem hiding this comment.
Might add note as to why large size is needed to test elision.
There was a problem hiding this comment.
might make sense to move the elide test into its own class, its currently in binop but abs is a unary
| @@ -565,7 +565,7 @@ array_negative(PyArrayObject *m1) | |||
| static PyObject * | |||
| array_absolute(PyArrayObject *m1) | |||
| { | |||
| if (can_elide_temp_unary(m1)) { | |||
| if (can_elide_temp_unary(m1) && !PyArray_ISCOMPLEX(m1)) { | |||
There was a problem hiding this comment.
Isn't this also going to fail for any user type that overrides abs too? #4730 comes to mind, and problems with a quaternion type.
There was a problem hiding this comment.
hm right, it shouldn't elide object arrays at all it only does that for unary not bianry probably I forgot to update both places.
There was a problem hiding this comment.
though abs(object-array) is again an object array regardless of what __abs__ does
There was a problem hiding this comment.
I'm referring to registering custom ufunc loops for new dtypes
There was a problem hiding this comment.
Issue number should have been #9003
|
In general, it's not safe to elide any op on a user-defined dtype - assumptions about commutivity and heterogeneity are both likely to be wrong. If you could detect the ufunc loop before calling it, then you wouldn't need to make those assumptions. Unfortunately, it seems that |
eb83723 to
7930414
Compare
|
disabled elision for all non-basic types for unary ops, they were already disable for binaries. |
As in binary operations, only elide unary operations on basic types to avoid odd interactions e.g. with user dtypes.
7930414 to
3abc112
Compare
|
Why are object arrays not safe to elide? |
|
they may be, but it is probably not worth the time thinking about it as they are likely to slow to profit anyway. |
|
@eric-wieser Are you OK with this? |
| @@ -359,7 +359,7 @@ can_elide_temp_unary(PyArrayObject * m1) | |||
| { | |||
| int cannot; | |||
| if (Py_REFCNT(m1) != 1 || !PyArray_CheckExact(m1) || | |||
| PyArray_DESCR(m1)->type_num == NPY_VOID || | |||
| PyArray_DESCR(m1)->type_num >= NPY_OBJECT || | |||
There was a problem hiding this comment.
Maybe just !PyTypeNum_ISNUMBER(m)? Do we want to include NPY_HALF here too? Or does that have ee->f loops which would cause this to fail?
If NPY_HALF is not ok, then I'd rather have an explicit enumeration of bad types here, with an explanation in a line comment of why each is bad.
There was a problem hiding this comment.
the ee->f loop is only used for non fast powers which is not elided (though it could be, albeit with not much performance gain as normal pow is very slow)
adding a test for that is a good idea though
There was a problem hiding this comment.
also I think that just tells the inner ufunc loop too cast per element, do the pow and cast back, the output buffer is actually still float16 so it can be elided.
There was a problem hiding this comment.
it is so, no test needed then
using ISNUMBER is much better, added that
There was a problem hiding this comment.
Should probably fix the release notes to say that elision is added for builtin numeric types
|
Thanks Julian. And thanks Eric for reviewin, and feel free to do the merge without waiting on me. |
complex abs() results in float so it cannot be elided.
Closes gh-9109