Repository navigation
Add native to_pandas byteorder conversion for ArrayWrapper Tables - #11294
Conversation
|
I have made the In fact one could have let Pandas take care of the entire conversion to native by |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
I like this very much, smaller, simpler code that leaves more stuff to pandas; great!
Only a total nitpick about one test.
Also a quick question for @taldcroft: is byteorder meant to be preserved in pickling? (if so, likely a separate issue, but good to look at it now). I can understand table initialization since by default it makes a copy (which would change the byteorder to native), but pickle is less obvious.
| m2 = func(m) | ||
| for attr in attrs: | ||
| assert getattr(m2.info, attr) == getattr(m.info, attr) | ||
| if (attr != 'dtype' or getattr(m.info.dtype, 'isnative', True) or |
There was a problem hiding this comment.
Total nitpick: could you make this 3 lines like
if (attr != 'dtype'
or getattr(m.info.dtype, 'isnative', True)
or func in (copy.copy, copy.deepcopy)):
We've moved to having operators at the start of a line (and this would seem clearer).
There was a problem hiding this comment.
Thanks, I'd been scratching my head a bit around where we want to put the operators now (and the original version had a lot of and nots...), but could not quickly spot an example...
| # Convert int64 to Int64, uint32 to UInt32, etc for nullable types | ||
| pd_dtype = pd_dtype.replace('i', 'I').replace('u', 'U') | ||
| out[name] = Series(column, dtype=pd_dtype) | ||
| pd_dtype = pd_dtype.capitalize().replace('i', 'I') |
There was a problem hiding this comment.
Another nitpick. I like the original version of this line as being more consistent, as in capitalize any i or u in pd_dtype using replace. If there is nothing logically wrong with the original then please revert.
There was a problem hiding this comment.
Sure; I just wanted to use capitalize as a neater method, but then realised that would not be enough to deal with the uints.
|
@mhvk - about pickling and byte order, I haven't thought about this before. Generally speaking I don't think of pickle as a serialization method that is rigorous like that, so I wouldn't expect byte order to be preserved. It is worth nothing that numpy arrays seem to convert from non-native to native byte order when pickled, so that's what I'd expect for astropy Table columns. |
|
OK on letting pickling be consistent with numpy! |
|
Failures unrelated, so merging... |
|
@astrofrog will 4.0.x support |
Add native to_pandas byteorder conversion for ArrayWrapper Tables
|
Since it's now the only thing holding up 4.0.5 I'm going to make a command decision to say we're going to push this to 4.0.6 - it will be in 4.2.1, though (since it backports cleanly there)(, which will make the changelog a bit confusing. I'll just put it in 4.2.1 for now in the 4.2.x branch. |
|
@eteq unless the |
|
@dhomeier - I'm going to re-milestone this to v4.0.7 as I can't figure out how to backport this correctly and have the tests still pass. Could you try and open a PR to the v4.0.x branch to manually fix this in that branch? |
Description
Follow-up to #11288
This handles some additional corner cases for
MixedArray-like tables, that were exempt from thebyteorder/isnativechecks since they were first converted to PandasSeriesobjects.Now checking and fixing byteorder first before any conversions to Pandas types, added an
ArrayWrapperwith non-native byteorder toMIXIN_COLSfor testing.Since
pickleandinit_from_classare not preserving byteorder in this case, their tests intest_info_preserved_pickle_copy_initare intercepted accordingly.