Refactor FITS compressed HDU to subclass ImageHDU

[mhvk]

The header for compressed images has a number of comments noting that its current design, as a subclass of Header does not work well at all, and that it would be good to refactor. This came up again while making Header more suitable for subclassing - it broke CompImageHeader (#9229). This issue is a reminder to revisit CompImageHeader.

It might be good to try to address compressed images more generally; see #3895

[saimn]

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):

[astrofrog]

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.

[mhvk]

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?

[astrofrog]

@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).

[saimn]

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 ?