DM-18036: Use Tables to read FITS #183
Conversation
There was a problem hiding this comment.
A couple of things. First, have you tested the relative speed of the Table version vs the fits version? If there's a difference, it might be fine, but I think we should know. Second, I'm confused about the string handling. Why are the strings not either true-strings or bags-o-bytes, and why is there a bunch of if/else blocks to test for this?
tests/test_readFitsCatalog.py
Outdated
|
|
||
| def testHDU2(self): | ||
| """Test reading HDU 2 with original order""" | ||
| config = ReadFitsCatalogTask.ConfigClass() | ||
| config.hdu = 2 | ||
| task = ReadFitsCatalogTask(config=config) | ||
| arr = task.run(FitsPath) | ||
| self.assertTrue(np.array_equal(arr, self.arr2)) | ||
| for name in self.arr1.dtype.names: | ||
| # Table form disk ends up being bytes and in memory is unicode |
There was a problem hiding this comment.
I think this should be "from disk". But I'm confused why there are these two options below? Why isn't it one or the other?
There was a problem hiding this comment.
I'll fix the typo.
I don't know why it does this, but the array constructed from a Table read from a fits file on disk has the strings as type bytes. If the array comes from a Table constructed from a FITS object in memory, they are unicode even if I try to force them to be bytes.
I didn't want to try to figure out if it was a bug or not, so just handle the one string like column separately. I know that's not particularly satisfying, but I'd prefer to figure out the difference without the pressure of the release blockage. I'll file a ticket if that's ok with you.
There was a problem hiding this comment.
At least put that in the comments, though is it possible that this could bite a user?
There was a problem hiding this comment.
Hold on ... what ticket is this associated with? It should be in the PR name.
There was a problem hiding this comment.
Gah. Very sorry about that. I was in a rush to make the PRs. This is DM-18036.
SimonKrughoff
force-pushed
the
tickets/DM-18036
branch
from
908cc2d
to
39dafe4
Compare
SimonKrughoff
changed the title
This also includes fixing the unit tests. Using astropy.table.Table also solves issues dealing with endianness of values coming from FITS files.
SimonKrughoff
force-pushed
the
tickets/DM-18036
branch
from
39dafe4
to
7b93ca0
Compare
|
@erykoff I know you already approved, but I simplified the test greatly. Feel free to have another look if you wish. |
This also includes fixing the unit tests.
Using astropy.table.Table also solves issues dealing with endianness of values coming from FITS files.