Unify promoter implementations and align behavior with NumPy - #120
Conversation
SwayamInSync
left a comment
SwayamInSync
left a comment
There was a problem hiding this comment.
This is great, I liked the refactor done.
Some nit-picks I mentioned in comments which isn't a blocker and I will handle them in the follow-ups.
Said that comparisons, logical_not, ldexp and frexp still fail when an output dtype is requested. They also go in a follow-up.
| Py_DECREF(ufunc); | ||
|
|
||
| return 0; | ||
| } No newline at end of file |
There was a problem hiding this comment.
| } | |
| } | |
adding trailing newline
| return Py_NewRef(self); | ||
| } | ||
|
|
||
| // TODO: Use PyArray_Conjugate when NumPy >= 2.5 is required. NumPy 2.4 |
There was a problem hiding this comment.
I think the real reason for the conj override, which is that NumPy 2.4's inherited generic.conjugate corrupts longdouble-backend scalars.
This was fixed in numpy/numpy#31067 and I think your PR which added the fix for byteswapping to error instead of segfault is also 2.5, so we should update the numpy-quaddtype's dependency from 2.4 to 2.5 (Otherwise have to create the override for all the scalar functions here, undesirable)
Also I think as we update the dependency of Numpy to >= 2.5, we can entirely delete this override as well.
There was a problem hiding this comment.
Small correction: I checked the release tags, and the byteswap fix isn't in 2.5.0. Your #32151 backport (#32221) landed in 2.5.2 and the scalar fix (#32254 → #32290) in 2.5.3. On 2.5.0, QuadPrecision(1.5).byteswap() still segfaults; on 2.5.2 it raises TypeError. So the floor should be numpy>=2.5.2 (or >=2.5.3 to include #32254), not just >=2.5.
|
I think the last commit responds to your comments. Thanks for the simplification. I also updated the CI config to match the updated minimum Python version from NumPy. |
SwayamInSync
left a comment
SwayamInSync
left a comment
There was a problem hiding this comment.
Great and I think we should ideally update the NPY_TARGET_VERSION in C files as well? I can also handle this in the follow-ups.
And users on Python 3.11 or NumPy below 2.5.3 will stop receiving new numpy-quaddtype releases, so the next changelog should mention it.
We don't do anything like
.rstfiles here hmmm... would it be worth it @ngoldbaum ?
|
CI failure is an unrelated flake. In it goes. Thanks for taking care of the followups! |
IMO towncrier is overkill for a project that's not very active. I'd say just add a section to the readme until you start getting annoyed by merge conflicts. |
🙂 (smiling in pain)
Yup will keep this in follow-up too |
|
Here "not very active" means the github issue/PR counter is below 10,000! At least IMO... |
2 participants
Fixes issues I noticed while looking over PR #118 today.
First, it unifies the promoter implementations to use a shared helper. Second, it updates the promoter logic to be more consistent with NumPy.
Currently, one can trigger errors due to ambiguous promotion. It's also not currently possible to do e.g.
np.add(quad, quad, dtype='float64'). This fixes both issues. See the tests. I also deleted the existing promoter tests because they're duplicative with the new tests.I used an AI model for help with this.