Avoid assuming byteswap on non-NumPy arrays in to_buffers#3845
Avoid assuming byteswap on non-NumPy arrays in to_buffers#3845X0708a wants to merge 5 commits intoscikit-hep:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files
🚀 New features to boost your workflow:
|
|
The documentation preview is ready to be viewed at http://preview.awkward-array.org.s3-website.us-east-1.amazonaws.com/PR3845 |
There was a problem hiding this comment.
Thanks for attempting this but this is not what the original issue is about. The issues asks to abstract byteswap inside the nplikes for all backends. Here are where the nplikes are implemented https://github.com/scikit-hep/awkward/tree/main/src/awkward/_nplikes. You'd need to understand what the nplikes do before implementing this.
|
That would also require an implementation of Edit: |
|
I reworked the PR to follow the intended design: Byteorder conversion now goes through nplike.byteswap(...) for nplike‑owned arrays (no assumptions about .byteswap() on buffers). |
There was a problem hiding this comment.
You are changing a million unrelated things here making it hard to review.
Also, for cupy you go through numpy (very bad if we have to copy from gpu to ram and back) but for jax you raise an error (inconsistent). It is possible to implement byteswap for both cupy and jax using view and reshape tricks or bit manipulation depending on the dtype's itemsize and I think that's the way to go.
Regarding, ak._util.native_to_byteorder, I'm still not sure about whether it should take the nplike as input or figure it out internally using nplike_of_obj. nplike_of_obj uses memoization so it's not slow to call it every time but on the other hand, if you already have the nplike extracted, it may be best to just pass into the function. The other route would be that the nplike actually implements native_to_byteorder and it's not a utility anymore but a method of each nplike. That's also a possible route.
You are also moving things around and changing things a lot in tests. We do not touch tests like that
|
I’ve minimized the diff and removed the unrelated test changes. CuPy byteswap is now GPU‑safe, and JAX byteswap is implemented using view/reshape/flip so it’s consistent. I also updated native_to_byteorder to infer the nplike when not provided, while still accepting an explicit one if available. Please take another look and let me know if you’d rather move native_to_byteorder onto the nplike itself. |
|
you are still changing dozens of unrelated lines, you have introduced a new function |
|
You are also making incorrect changes in |
|
Refactored to abstract byteswap inside nplike layer for all backends, removing assumption that arrays implement .byteswap(). |
918bf2c to
d4b10c2
Compare
|
There are major things missing here. Let's take it step by step. There should be a |
|
Hi, |
|
For the numpy nplike, going through the custom byteswap implementation is expensive. Numpy nplike should implement byteswap using |
|
Pushed follow up updates - Kept the generic nplike byteswap path for non-NumPy backends, with correct complex component. Updated virtual byteswap to dispatch through nplike (no direct array-method assumption). Split tests by backend- Removed- also- |
|
Pushed a JAX follow-up: complex64/complex128 byteswap is explicitly marked unsupported for JAX and those two JAX test cases are skipped. Other JAX byteswap dtypes remain covered, and NumPy/CUDA coverage is unchanged. |
|
Since we are abstracing |
|
Ran benchmarks for both reshape+view and bitwise across NumPy and JAX for all dtypes. JAX NumPy's native .byteswap() is clearly the fastest so that should stay as the fast path. For the generic implementation, reshape+view generally beats bitwise on 64-bit and complex types , bitwise wins sometimes on 16/32-bit but it's not consistent enough to justify the extra complexity. |
|
It would be good to see the benchmark (a gist maybe) for clarity too. Cupy should be tested too if possible in the benchmark. For jax, what do you mean they fail the roundtrip? It shouldn't fail if you apply the reversal in the real and imaginary part separately. What's the failure? |
|
Benchmark script: https://gist.github.com/X0708a/84142e1e7ba0ef4e1b8c7acc05dc3ccd |

Fixes #2673
This PR removes the assumption that all array-like buffers implement
.byteswap()in theto_bufferspath. The byteorder conversion is nowexplicit for NumPy-backed buffers, avoiding unsafe assumptions across
ndarray implementations.
Tests updated accordingly.