Repository navigation
Fix check for byteorder swap in Table.to_pandas() - #11288
Conversation
dhomeier
added
Bug
copyedited
zzz 💤 merge-when-ci-passes
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Ha, didn't know about isnative! That seems exactly what is needed.
The change and test look great, but could you add a changelog entry? This sounds like something we should backport, so I set the milestone to 4.0.5.
| if (hasattr(out[name].dtype, 'byteorder') | ||
| and out[name].dtype.byteorder not in ('=', '|')): | ||
| out[name] = out[name].byteswap().newbyteorder() | ||
| if not getattr(out[name].dtype, 'isnative', True): |
There was a problem hiding this comment.
Looking at this again, why are we using getattr rather than just getting not out[name].dtype.isnative? Are there cases where the dtype does not have that attribute? (I'm actually confused that the dtype could possibly not have 'byteorder' as well....)
There was a problem hiding this comment.
Yes, at least some of the specific integer dtypes apparently have neither byteorder nor isnative, probably the reason for the original if (hasattr(out[name].dtype, 'byteorder'). Otherwise causing failures e.g. in
FAILED lib/python3.8/site-packages/astropy/table/tests/test_table.py::TestPandas::test_nullable_int[32-] - AttributeError: 'Int32Dtype' object has no attribute 'isnative'
FAILED lib/python3.8/site-packages/astropy/table/tests/test_table.py::TestPandas::test_nullable_int[64-u] - AttributeError: 'UInt64Dtype' object has no attribute 'isnative'
FAILED lib/python3.8/site-packages/astropy/table/tests/test_table.py::TestPandas::test_nullable_int[64-] - AttributeError: 'Int64Dtype' object has no attribute 'isnative'
FAILED lib/python3.8/site-packages/astropy/table/tests/test_table.py::TestPandas::test_masking[True] - AttributeError: 'Int64Dtype' object has no attribute 'isnative'So I assumed that endianness did not apply here just as for '|' byteorders, but on a second look, being integer types, I am not so sure any more they will really end up in the right format.
There was a problem hiding this comment.
Maybe those are pandas dtypes? They don't look like numpy... Checking, indeed, they are specific to pandas.
Anyway, since you are not changing the way the code works here, I think it makes sense to just merge this.
But let me ping @taldcroft so he can think of whether, more generally, this is working as it should here.
There was a problem hiding this comment.
The above is the case for MaskedColumn conversions, which for integer types first convert to a Pandas Series, and those do not have the dtype attributes. They could instead be accessed under dtype.numpy_dtype, but converting a non-native column would already fail before that:
>>> out = Series(column, dtype='>I2')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/opt/lib/python3.8/site-packages/pandas/core/series.py", line 249, in __init__
dtype = self._validate_dtype(dtype)
File "/opt/lib/python3.8/site-packages/pandas/core/generic.py", line 253, in _validate_dtype
dtype = pandas_dtype(dtype)
File "/opt/lib/python3.8/site-packages/pandas/core/dtypes/common.py", line 1763, in pandas_dtype
npdtype = np.dtype(dtype)
TypeError: data type '>I2' not understoodSo that case is obviously not covered by the tests, but would fail for non-native, masked integer columns.
Not sure why the integer types require this special treatment, but at least it will raise a clear exception early on.
Perhaps the whole byteorder check could instead be done first thing on the original column before checking for MaskedColumn etc....
|
Thanks; the |
|
I think this is because in pandas for real data, one uses |
|
@taldcroft - I think right now we may not be dealing quite properly with pandas masked integer columns, though my guess is that we're just happily keeping them in whatever byte order they have, which is probably OK. Anyway, I think it is fine to let those who use pandas try to fix any inconsistencies. |
|
@mvhk yes, I think floating point columns with masked elements were left in original byteorder, which is fixed by #11294; this probably would lead to problems somewhere downstream in actually working with the dataframes, but is obviously rare enough to have never become an issue. For the integer types the |
Fix check for byteorder swap in Table.to_pandas()
Fix check for byteorder swap in Table.to_pandas()
Description
Fix #11286 –
Table.to_pandas()is supposed to export columns always in native byteorder, i.e. swap if, and only if,dtype.byteorderis non-native. This failed in some cases with data identifying e.g. asbyteorder='<'on a little-endian system, which was not recognised as'='and thus incorrectly swap.This PR attempts to improve identification of non-native byteorder and extends the tests to probe '<', '>', '=' independently.
EDIT: Corrected auto-close syntax