Repository navigation
implement reading / writing .fits.bz2 files - #3789
Conversation
|
Thanks a bunch Paul-- I haven't seen too many bz2-compressed FITS files in a while, but I'm sure (obviously) that they're out there and I appreciate that it might be a better choice in some cases, so I definitely have no problem with adding support for this. I'll follow up with a few comments. One thing I'll immediately note is that your change seems to have caused a few tests to fail--did you run the tests locally? |
There was a problem hiding this comment.
Ah, I see what you're saying about the indentation here. This line should be at the level of indentation it was at previously--we should try seeking to the beginning in all cases (compressed and uncompressed). It's the comment that was mis-indented.
It would probably be clearer if there were a newline before the comment, and the comment were dedented one level.
|
Unfortunately it looks like you may also have to add something here, to support resizing when updating bz2-compressed files: https://github.com/astropy/astropy/blob/master/astropy/io/fits/hdu/hdulist.py#L963 There can just be an elif there for (note: I say "unfortunately" because this is all code that should probably be in the file module, instead of where it is currently, which is hard to find. Looks like a good target for refactoring) |
|
Thanks for the feedback Erik! I thought I'd ran the tests locally OK but maybe I missed something or it was version specific. Anyway, I've fixed up the issues today and added some tests to test the bzip2 functionality. I think it's all good, I just added some more tests to try and nudge the test coverage up (was getting -0.0 at the last one), will wait to see how they all go. |
There was a problem hiding this comment.
One little nitpick is to include the issue number here. After the period just add a space followed by [#3789] (per the example of other changelog entries)
|
Looks great! Other than that one tiny nitpick I think this is good. |
|
Thanks Erik - I really appreciate the guidance! I fixed that and also added a short section to the documentation regarding compressed files - I for one hadn't actually realized there was gzip support before I looked at the code. I wasn't sure where to put this in the docs, if there's somewhere better or you have any suggested edits to it, please do let me know. |
|
Thanks for the addition to the docs too. Looks good to me. |
implement reading / writing .fits.bz2 files
I added support for bzip2 (.bz2) compression to io.fits in file.py, following the way that gzip is supported.
I appreciate that in many cases it's better to use FITS compressed data units, and that the module already supports gzip, but I find myself working with a lot of fits.bz2 files (on the data I have, bzip2 achieves a significantly better compression ratio per unit cpu time than gzip). I also appreciate that it might be perceived as a slippery slope towards wanting support for every possible compression scheme... However, the bzip2 module is part of the python standard library, the changes in io.fits file.py are fairly minimal and clean, and there is significant .fits.bz2 use in the wild.
One thing that I wasn't sure about - the indent of the self._file.seek(0) at line 442/450 did not match the comment that explained it - the comment was inside the if block, the call wasn't. I don't see how the file couldn't be at the start for the file object that had just been directly opened anyway, and this breaks if you call seek on a bzip2 file object other than in read-only, so I moved it into the if block to match the comment.
Finally, I've been lurking a while, this is my first commit, comments / feedback welcome... :-)