ENH: add no-op fallback in np.byteswap for types with no byte order - #32150
Conversation
ngoldbaum
added
01 - Enhancement
component: numpy.dtype
component: numpy.strings
ngoldbaum
added
the
56 - Needs Release Note.
|
I marked this as draft because I want to do this after #32151 merges now. This PR will contain the "new" allowed behaviors, once we decide on them. I think only error make sense for a backport. |
27f32fd to
cf212c4
Compare
| * ``ndarray.byteswap`` and ``numpy.byteswap`` now support dtypes that have no | ||
| defined byte order (e.g. when ``dtype.byteorder == "|"``). This includes NumPy's | ||
| string dtypes. Such values are left unchanged, including when they occur in | ||
| structured dtypes. |
There was a problem hiding this comment.
This needs a different format with a title right? + numpy.byteswap does not exist.
| * ``ndarray.byteswap`` and ``numpy.byteswap`` now support dtypes that have no | |
| defined byte order (e.g. when ``dtype.byteorder == "|"``). This includes NumPy's | |
| string dtypes. Such values are left unchanged, including when they occur in | |
| structured dtypes. | |
| Byteswapping dtypes without a defined byte order is now supported as as no-op | |
| ----------------------------------------------------------------------------- | |
| ``ndarray.byteswap`` now supports dtypes that have no defined byte order | |
| (e.g. when ``dtype.byteorder == "|"``). This includes NumPy's string dtypes. | |
| Such values are left unchanged, including when they occur in structured dtypes. |
There was a problem hiding this comment.
This needs a different format with a title right?
You don't need to use a title, no. We also have all_bullets = false in our towncrier config:
Line 113 in 5799ed4
So if I explicitly add a bullet like this, it'll render as one of a number of bulleted bugfixes. I don't think this is worth a whole heading in the release notes, personally.
Thanks for catching my using the wrong name!
There was a problem hiding this comment.
Ah I was not aware of the no-heading formatting. numpy.byteswap is the only valid comment then here.
There was a problem hiding this comment.
we should probably add some text about it to the README we have in the release notes folder.
There was a problem hiding this comment.
I also loosened the text about string dtypes here. Of course the UCS-4 unicode dtype is byteswappable.
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Had a decent go-over and this looks all fine (I would not have picked up on the VOID/etc asserts...).
ikrommyd
left a comment
ikrommyd
left a comment
There was a problem hiding this comment.
If CI is happy, I'm happy :)
ngoldbaum
removed
the
56 - Needs Release Note.
|
Thanks for the review all! |
3 participants
PR summary
This adds a no-op fallback for dtypes that declare no particular byte order (
"|"). IMO this is a sensible default for these dtypes.To make e.g.
QuadDTypework properly here we'll need to add an API hook that allows a user dtype to define a byteswapping operation. Punting on that for now.AI Disclosure
I used an AI model to assist with debugging this change.