| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Thanks, seems right! FWIW I think it is OK to change PyArray_ISNUMBER at the level of PyTypeNum_ISNUMBER (doing it locally for me didn't break NumPy tests at least, and type number checks can often be legacy). If it breaks any code it is probably unearthing bugs. That said not entirely familiar with these macros so maybe wait for someone else. |
Sorry, something went wrong.
|
Nice, thanks for looking into this! I think it is probably good enough to reject only parametric dtypes here? I.e. with other dtypes we assume that things are fine. |
Sorry, something went wrong.
|
Just pushed a new fix which doesn't check type_num but instead checks for presence of the NPY_DT_PARAMETRIC flag. I'm checking the flag manually rather than through NPY_DT_is_parametric to avoid pulling in dtypemeta.h. |
Sorry, something went wrong.
|
I am sorry, I remembered last night late and re-remembering only right now... But what slipped my mind completely is that the Py_NUMBER macro was just wrong, it should include the > 0 check that you correctly added. (I would be happy to broadening it up to the newer NPY_DT_is_numeric() && !parametric, but I am not fully confident that it is guaranteed correct always, it might need a flag to say "go ahead an use in-place for homogeneous operations".) EDIT: If it's annoying, I may just push it later. |
Sorry, something went wrong.
|
No problem, I will try changing PyTypeNum_ISNUMBER and let you know what happens. |
Sorry, something went wrong.
|
The macro fix seems to work equally well, both for my custom dtype test suite and the numpy test suite. |
Sorry, something went wrong.
|
Thanks. Yeah, it seems like anything would be guess-work, while usually true, weird promotion could always happen. @SwayamInSync maybe you have time for a quick look. I pushed one more hot-fix for the single case that looked like it might cause regressions here. I.e. that quaddtype_arr.conjugate() works via the ufunc. Now, it seems for NumPy conjugate() actually just returns self (although I think a view should be OK). But, I think that fix isn't back-portable. For a better fix, I think we could:
The above would mean that quaddtype_arr.conjugate() would return self. That is a theoretical regression for user defined new-style complex dtypes. |
Sorry, something went wrong.
That's related to a different issue which also affects my complex fixed-point dtype: there is no way to inform numpy that my type is complex, so it assumes it is real and uses the trivial implementation of .real (return self), .imag (return zero) and .conj() (return self). I wouldn't mind making a fix for that too, but would need some guidance on how to proceed. This might require another dtype flag? |
Sorry, something went wrong.
#30984 included ufuncs for real and imag, so for now you can register them via PyUFunc_AddLoopsFromSpecs as in the docs. These would be then used for your dtype. A flag to use defaults would still be nice though. |
Sorry, something went wrong.
|
Good to know! Is there a similar ufunc for conj? |
Sorry, something went wrong.
|
Yes pretty sure "conjugate" is the one! That is a very "real" ufunc so you should be able to register it directly even IIRC. |
Sorry, something went wrong.
| { | ||
| if (PyArray_ISCOMPLEX(self) || PyArray_ISOBJECT(self) || | ||
| PyArray_ISUSERDEF(self)) { | ||
| PyArray_ISUSERDEF(self) || !NPY_DT_is_legacy(PyArray_DESCR(self))) { |
There was a problem hiding this comment.
Thanks @seberg this works well. LGTM
Also noticed in quaddtype's we don't have test for quad_arr.conj() (we use np.conj which directly through the ufunc dispatch machinery and works)
Sorry, something went wrong.
|
Thanks for having a look @SwayamInSync, I'll put this in then. I'll try to follow up with something that makes this work better for quaddtype in the future (e.g. also returning a view). I think we can backport this @charris, but if it doesn't matter to @MaartenBaert then I don't mind either way. |
Sorry, something went wrong.
#31193) Co-authored-by: Maarten Baert <maarten.baert@keysight.com> Co-authored-by: Sebastian Berg <sebastianb@nvidia.com>
BUG: incorrect temp elision for new-style (NEP 43) user-defined dtypes (#31193)
| Back | FazBrowse Home | New Git URL |
PR summary
can_elide_temp() in temp_elide.c incorrectly identifies new-style user-defined dtypes as numeric types eligible for in-place buffer reuse. For large arrays this silently rewrites a*a + b*b into (a*a) += (b*b), which raises a TypeError when the result dtype of the in-place add does not match the pre-allocated buffer.
I encountered this bug while implementing a custom fixed-point dtype, which automatically increases its bit width on every operation as required to avoid overflow. Since addition increases the bit width by one, attempting to reuse the temporary buffer results in an incompatible destination dtype being provided to the add ufunc and raises a TypeError.
The root cause is that PyArray_ISNUMBER evaluates to true for new-style user dtypes since it checks (type) <= NPY_CLONGDOUBLE and type_num = -1 for these types.
This could be fixed either in PyArray_ISNUMBER (probably more correct, but may have unexpected side effects) or with a minimal extra check in can_elide_temp (the approach used by the PR). Let me know if you'd like me to fix PyArray_ISNUMBER instead.
AI Disclosure
This bug was identified and fixed by Claude Sonnet 4.6, and then reviewed by me.
Full AI-generated explanation: https://gist.github.com/MaartenBaert/7540176c4005d26a0b292baefbec8519