Skip to content

Always convert to native byte order with FITS Table.read() #4069

Closed

Description

@taldcroft

Having non-native byte order in tables causes a number of problems which have been documented in #4034 and #1156. I just ran into another weird issue writing HDF5 files using PyTables.

In #1156 there was general consensus that always converting to native byte order when using Table.read() for FITS files would be a good thing. The direct astropy.io.fits interface would be available if that behavior was not desired.

@embray provided a small code snippet that should do the job:

sys_byteorder = ('>', '<')[sys.byteorder == 'little']
if col.dtype.byteorder not in ('=', sys_byteorder):
    col = col.byteswap().newbyteorder(sys_byteorder)
    # Do something with col array...

This issue is to formally decide to go forward with this plan and then do it!

Activity

  1. added this to the v1.1.0 milestone on Aug 12, 2015
  2. embray commented on Aug 12, 2015

    @embray
    Member

    I don't think I'm crazy about this idea, and I'll tell you why--one thing I'm trying to work towards is using the Table class as the sole interface for FITS tables at all levels. Yes, Table.read() would still work, but also accessing the data in a FITS HDU would return a Table. There's no advantage to having a separate "low level" interface that isn't very good anyways, and it's a pain to maintain (just, also, a pain to transition).

    Being able to read the big-endian arrays directly off disk is pretty important for many applications, especially any involving large files. Yes it's inconvenient that FITS uses big-endian, but oh well. I don't see how you plan to manage using two different methods to load a FITS table into a Table object, where one also loads all the columns into memory and byteswaps them. I can see the bug report now: "When I do hdu.data I can read columns out of my table fine, but when I use Table.read() I run out of memory."

    I think a better approach to this is to figure out why PyTables can't handle Numpy arrays with big-endian data. Since Numpy already provides the requisite information about endian-ness, software that reads data in from Numpy arrays should know what to do with buffers that contain non-native values. My guess is that they also want to the underlying array buffers directly, but oh well--they should check the array descriptor and copy byteswapped values into their own buffer. Numpy's API allows this.

  3. taldcroft commented on Aug 12, 2015

    @taldcroft
    MemberAuthor

    On the one hand I see your point of not applying band-aids in astropy Table and instead fix upstream bugs. On the other hand, those upstream bugs related to byte order seem to be pretty common and can be very difficult to diagnose. I've accidentally run into these problems on two separate occasions and both times it was frustrating.

    For the particular case of PyTables, there were actually two issues:

    • When I had a table with mixed byte order (I added a new column to a FITS table) then PyTables just refused with a message like "Sorry PyTables doesn't support mixed byte order yet". Somehow this implies to me that it is not going to be fixed any time soon with a small patch.
    • When I had a table with all big-endian, the code ran to completion but wrote out a corrupted table. When I ran the same code but converted the table to all little-endian there was no problem. Yes, I need to do the right thing and file an issue, but I was in a rush and just wanted things to work. That's the default situation for most astronomy users.

    That's why I think there should be a "simple interface" for FITS where if you read the whole table into memory it automatically does the swap to native order. In this regime it will reduce weird bugs without a big penalty in performance.

    A middle ground would be a Table method like set_native_byteorder() that would update columns in-place to native byte order. Doing this in place has the advantage that each column can be updated individually, thus reducing the temporary increase in memory for big tables. I would tend to use this method pretty frequently for any production code.

  4. embray commented on Aug 13, 2015

    @embray
    Member

    Another middle-ground approach I'd be amenable to is when using memmap=False to always (for tables or arrays) immediately byteswap the data to native byteorder. If the array is all ready into memory anyways there's no real advantage to keeping it in big-endian.

    I would like to investigate the issues with PyTables though. I can't imagine it would be a significantly deep problem to fix--it probably just doesn't come up often anymore.

  5. embray commented on Aug 13, 2015

    @embray
    Member

    Another thing I find interesting--the Table class treats each column as a separate array (even in the case of reading from a FITS file, where each array is actually a slice of a recarray). So when you pass it in to PyTables, at what stage is it complaining about "mixed byte order"? Does it just look at the dtype of the table? Or is trying to read in one row at a time and finding that the rows contain data in mixed byte order?

  6. taldcroft commented on Aug 13, 2015

    @taldcroft
    MemberAuthor

    I was converting to a structured array prior to passing to PyTables:
    https://github.com/sot/fid_drift_mon/blob/master/aimpoint_drift/get_asol_aimpoint.py#L50
    In this asol is a Table with mixed byte-order columns. Here is a simple demo:

    In [2]: import tables
    In [3]: h5 = tables.open_file('junk.h5', mode='a')
    In [5]: t = Table([[1,2], [3,4]], dtype=('<i4', '>i4'))
    In [6]: ta = t.as_array()
    In [7]: h5.create_table(h5.root, 'data', ta, "title")
    ---------------------------------------------------------------------------
    NotImplementedError                       Traceback (most recent call last)
    <ipython-input-7-fbadd152f0ca> in <module>()
    ----> 1 h5.create_table(h5.root, 'data', ta, "title")
    
    /Users/aldcroft/anaconda/lib/python2.7/site-packages/tables/file.pyc in create_table(self, where, name, description, title, filters, expectedrows, chunkshape, byteorder, createparents, obj)
       1058                       description=description, title=title,
       1059                       filters=filters, expectedrows=expectedrows,
    -> 1060                       chunkshape=chunkshape, byteorder=byteorder)
       1061 
       1062         if obj is not None:
    
    /Users/aldcroft/anaconda/lib/python2.7/site-packages/tables/table.pyc in __init__(self, parentnode, name, description, title, filters, expectedrows, chunkshape, byteorder, _log)
        844                     self._v_recarray = nparray
        845                 self.description, self._rabyteorder = \
    --> 846                     descr_from_dtype(nparray.dtype)
        847 
        848         # No description yet?
    
    /Users/aldcroft/anaconda/lib/python2.7/site-packages/tables/description.pyc in descr_from_dtype(dtype_)
        801             if fbyteorder not in ['|', byteorder]:
        802                 raise NotImplementedError(
    --> 803                     "structured arrays with mixed byteorders "
        804                     "are not supported yet, sorry")
        805             fbyteorder = byteorder
    
    NotImplementedError: structured arrays with mixed byteorders are not supported yet, sorry
    
  7. taldcroft commented on Aug 13, 2015

    @taldcroft
    MemberAuthor

    Another middle-ground approach I'd be amenable to is when using memmap=False to always (for tables or arrays)

    Sounds good to me.

  8. embray commented on Aug 14, 2015

    @embray
    Member

    I forgot about Table.as_array()--that will always make a copy anyways, right? We could also make that return types all in native byteorder (or at least make it an option to)?

  9. embray commented on Aug 14, 2015

    @embray
    Member

    ^^^ I'll have a PR up for this in a bit.

  10. astrofrog commented on Sep 30, 2015

    @astrofrog
    Member

    @taldcroft - is this still needed now we have #4080?

  11. taldcroft commented on Sep 30, 2015

    @taldcroft
    MemberAuthor

    Resolved by #4080.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions