-
Notifications
You must be signed in to change notification settings - Fork 133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Endianness support is missing #97
Comments
FITS as an astrophysics-specific big-endian format is annoying indeed. Your A few thoughts:
|
I agree with this. If and when we get to adding something like |
While non-native byteorder is useful for serialization and less for in-memory exchange. As of now the implicit assumption seems to be that the
We do need to acknowledge that for serialization/networking having a fixed(non-machine native) endian is useful. I personally would consider that to be outside the scope(as the main purpose is fast in-memory exchange for computing). We can always run explicit byteswaping(which is needed anyway as most frameworks need that) in the scenario of serialization. |
I think the question here is whether to automatically byteswap or not. If users get FITS or HDF5 file with non-native byteorder and byteswapping is not done upon loading that data into memory (as is the case now), then there are two options:
I think I have a slight preference for (2). |
I also prefer (2) as it simpler |
That's what NumPy does right now: >>> import numpy as np
>>> x = np.array([1,2,3], dtype='>i2')
>>> np.from_dlpack(x)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: DLPack only supports native byte swapping. And what others should also do IMO. I don't think it is unreasonable/unintuitive to ask the user to first convert the data to native byte ordering before exporting. So, +1 for (2) |
Thanks for the discussion and the consensus during my absence. I agree with you all that
|
cc: @seberg (in case you have other thoughts since you're playing with endianness 🙂) I think this issue can be closed by a simple doc update, since no change is required on the DLPack side. If no one else is interested in submitting a PR before the end of this month, I'll give it a shot. |
No opinion, to me it seems like probably a valid but low-prio addition. And it is simple in that if you flag it on the dtype you get the "unsupported dtype" error for free. |
Complaints on endianness has been something I've recurrently seen (ex: CuPy cupy/cupy#3652 and mpi4py mpi4py/mpi4py#177), and I anticipate at some point we'd start receiving bug reports on this. Apparently there is at least a few communities out there (astropy and hdf5) that prefer (or could work with) non-native (that is, big) endianness data. This causes problems if two libraries exchange but do not communicate the endianness for how to interpret the data.
I suggest two possible solutions:
DLDataType
as a new struct member:DLDataType::code
to make it carry this information:cc: @tqchen @rgommers @tirthasheshpatel
The text was updated successfully, but these errors were encountered: