Repository navigation
More careful handling of masked values in FITS tables - #11222
Conversation
mhvk
added
table
io.fits
Bug
API change
0401075 to
58d0cb5
Compare
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Some clarifications if needed.
| finally: | ||
| hdulist.close() | ||
|
|
||
| # Check if table is masked |
There was a problem hiding this comment.
I thought we should just do away with the masked table altogether, as it is more or less deprecated.
| masked = mask = False | ||
| if col.null is not None: | ||
| mask = col.array == col.null | ||
| # Return a MaskedColumn even if no elements are masked so |
There was a problem hiding this comment.
Does this make sense? Or should we always check if any masked values are present?
There was a problem hiding this comment.
The current solution makes sense.
| assert np.all(t['c2'].mask == np.array([False, False])) | ||
| assert np.all(t['c3'].mask == np.array([False, False])) | ||
| assert np.all(t['c4'].mask == np.array([False, False])) | ||
| assert not hasattr(t['c2'], 'mask') |
There was a problem hiding this comment.
A consequence of no longer returning a table with masked=True. I think worth it.
|
cc @taldcroft for the table link, @astrofrog for the fix needed for timeseries |
saimn
left a comment
saimn
left a comment
There was a problem hiding this comment.
A little comment about the changelog, otherwise seems good to me (and a very nice improvement) !
| masked = mask = False | ||
| if col.null is not None: | ||
| mask = col.array == col.null | ||
| # Return a MaskedColumn even if no elements are masked so |
There was a problem hiding this comment.
The current solution makes sense.
|
Thanks! I'll use this opportunity to test the revived `[skip ci]' option... |
saimn
left a comment
saimn
left a comment
There was a problem hiding this comment.
Thanks @mhvk, looks good to me. I will leave it open if @taldcroft or @astrofrog want to have a look.
As noted in #11194, writing masked float and string columns should be possible, by using
np.nanand null string as null values, respectively, but this was broken in the transition of masked tables to tables able to containMaskedColumn. This is fixed here (and improved, also fixing complex).Furthermore, upon reading tables, those FITS null values are properly recognized and used to initialize a
MaskedColumn.I wasn't quite clear whether the latter should be labelled a bug or API change, but as I worried about breaking code in a bug-fix release, conservatively decided on API change, hence the 4.3 milestone. Obviously, happy to change this.
fixes #11194.