Refactor FITS compressed HDU to subclass ImageHDU #9238

Activity

1. mhvk

   added

   io.fits

   Package-expert

   Effort-medium

   Refactoring

   on Sep 15, 2019
2. saimn commented on Sep 15, 2019

   

   saimn

   on Sep 15, 2019

   Contributor

   More actions

   I think that the root of the issues mentioned in the comments is the need to change CompImageHDU to subclass ImageHDU (instead of BinTableHDU), and handle internally a BinTableHDU (or just its header?), while the opposite is done currently:

   astropy/astropy/io/fits/hdu/compressed.py

   Lines 386 to 388 in fea594d

   	# TODO: Fix this class so that it doesn't actually inherit from BinTableHDU,
   	# but instead has an internal BinTableHDU reference
   	class CompImageHDU(BinTableHDU):

   astropy/astropy/io/fits/hdu/compressed.py

   Lines 1874 to 1877 in fea594d

   	# TODO: This was copied right out of _ImageBaseHDU; get rid of it once we
   	# find a way to rewrite this class as either a subclass or wrapper for an
   	# ImageHDU
   	def _dtype_for_bitpix(self):

   👍React with 👍3Reacted by Marten van Kerkwijk, Michael Seifert and Thomas Robitaille
3. saimn

   changed the title [-]Refactor FITS compressed image headers (CompImageHeader)[/-] [+]Refactor FITS compressed HDU to subclass ImageHDU[/+] on Nov 3, 2022
4. astrofrog commented on May 4, 2023

   

   astrofrog

   on May 4, 2023

   Last edited by astrofrog

   Member

   More actions

   I have been experimenting with refactoring CompImageHDU to inherit from ImageHDU instead of BinTableHDU, and I am mostly there (it allows a net removal of ~500 lines of code and does simplify things and in my opinion makes things more robust).

   However, one of the things where we might have to break API related to the original issue here is that as CompImageHDU wouldn't really be needed anymore, and would be nice to remove, we would no longer emit warnings when setting keys on CompImageHDU.header such as tested here:

       def test_compression_header_insert(self):
           with fits.open(self.data("comp.fits")) as hdul:
               imghdr = hdul[1].header
               tblhdr = hdul[1]._header
               # First try inserting a restricted keyword
               with pytest.warns(UserWarning, match="Keyword 'TFIELDS' is reserved") as w:
                   imghdr.insert(1000, "TFIELDS")

   because we wouldn't be subclassing Header. Instead, we could emit those warnings when the file is actually written out (which is the point at which the header is converted to a binary table header in my refactor).

   Would we be ok with this change in API or is it crucial that we preserve the warnings at the point where the header keywords are set? (in which case we would need to keep a simplified version of CompImageHDU).

   I don't think making this change would really break people's code as such because it just moves the point at which the warning is emitted, but just wanted to check if there are any reasons this could be an issue.
5. mhvk commented on May 4, 2023

   

   mhvk

   on May 4, 2023

   ContributorAuthor

   More actions

   Do you mean that CompImageHeader is not really used any more? I tried to have a quick look at the PR, but got stranded as I don't really have a good sense of FITS internals. Still, on the very unlikely chance this is a useful question: it looked like one had access to .bintable.header, so could the warning not be preserved?
6. astrofrog commented on May 4, 2023

   

   astrofrog

   on May 4, 2023

   Member

   More actions

   @mhvk - in my refactor, we can completely remove CompImageHeader (don't look at the PR yet as it's not quite ready and doesn't fully remove that class but will do soon). Instead, CompImageHDU just has a regular Header as it's .header attribute and when the file is about to be written out, the header is converted on-the-fly to the binary table header.

   The class used to be needed because the compressed and decompressed header were kept in sync in real time, even though this wasn't strictly needed because the compressed header was private anyway. But it meant that one could do validation as soon as the header was changed. If we now just have a regular Header, and convert it on-the-fly at write time, that's when the warning will happen.

   As a side note, you mention .bintable.header but note that it is ._bintable.header which is private, there will be no way to access the binary table header (nor was there before either).

   👍React with 👍1Reacted by Marten van Kerkwijk
7. saimn commented on May 4, 2023

   

   saimn

   on May 4, 2023

   Contributor

   More actions

   Maybe a better idea would be to generalize this kind of check to the other HDU classes. Currently with ImageHDU or BinTableHDU you can modify/set reserved keywords but this will have no effect and those changes will be lost when writing the file because those keywords are set from the data. So maybe we could move this verification system to Header and have each HDU class specify a list of reserved keywords.
   Need for further thought as I don't have the details in mind, and this should be done later anyway, but maybe for now it's worth keeping the CompImageHeader subclass with this keyword checking ?
8. saimn

   closed this as completedin #15474on Sep 30, 2024