Refactor CompImageHDU to inherit from ImageHDU

[astrofrog]

This pull request addresses https://github.com/astropy/astropy/issues/9238 by refactoring CompImageHDU to inherit from ImageHDU instead of BinTableHDU. This includes some subtle API changes that cannot be warned about in advance using deprecation warnings or future warnings, so this PR, if accepted, would definitely warrant a major version bump. Ideally this would go in v6.0 but it depends on whether we can achieve consensus and stamp out remaining issues.

This PR isn't 100% ready (see 'Outstanding issues' below and FIXME comments in code) but I wanted to try and open this now in case it is something we can consider getting in before the v6.0 feature freeze. I doubt that the outstanding issues would cause significant code changes so it should be safe to already review things as-is.

Background

Compressed image HDUs are stored in FITS files as binary tables. The CompImageHDU was therefore developed as a a subclass of BinTableHDU. However, that implementation is quite complex because it tries to look to users like an ImageHDU - for example .header is meant to be the image header, and .data is meant to be the decompressed image data. To quote comments from the code:

# TODO: The difficulty of implementing this screams a need to rewrite this module

Bypasses `BinTableHDU._writeheader()` which updates the header with
metadata about the data that is meaningless here; another reason
why this class maybe shouldn't inherit directly from BinTableHDU...

Issue https://github.com/astropy/astropy/issues/9238 by @mhvk also suggested that CompImageHDU would be simpler if it inherited from ImageHDU.

This PR does that refactoring, and represents CompImageHDU as an ImageHDU subclass that internally stores a BinTableHDU representing the compressed data. Instead of always keeping the image and table header in sync, headers are only converted at I/O time which should in principle make things faster.

Overall, this results in a few hundred lines of code being removed. For now CompImageHeader is kept to avoid breaking API so that warnings about reserved keywords are emitted as soon as they are set. However, if we would be happy to 'break' this behavior and only warn at I/O time or when calling verify, we could get rid of the custom header subclass. If we want to keep the behavior, we can nevertheless try and avoid code duplication with the Header class by ssimplifying the header validation (see https://github.com/astropy/astropy/pull/15293).

API changes/fixes

I have tried to minimize API changes as much as possible. However, it is impossible to not have any changes at all:

• With this PR, a compressed image HDU chdu will no longer return True for isinstance(chdu, BinTableHDU). There's no way to avoid this and also no way to do this with a gradual deprecation. However, I would hope that this is rare and that if anything people would do isinstance(chdu, CompImageHDU) which will still be True.

• The following options have changed defaults in the CompImageHDU initializer to match ImageHDU:

  • uint=True replaces uint=False (I need to check if I can come up with an example that shows the difference here)
  • scale_back=None replaces scale_back=False (in practice I think this won't change anything)

• The checksum on the main header is now the image checksum not the binary table checksum (I believe this was a bug: https://github.com/astropy/astropy/issues/14753)

Modified tests

I've tried to change tests as little as possible but there were some unavoidable changes:

• Many tests (probably non-optimally) were checking the contents of chdu._header which no longer exists (the equivalent is chdu._bintable.header.
• test_compressed_header_missing_znaxis has been removed - this checked that removing ZNAXIS or ZBITPIX from the internal binary table caused accessing the compressed data property to fail. This doesn't seem needed because it relies on modifying private attributes on CompImageHDU and we now also generate the compressed header on-the-fly so there is no opportunity to remove the keywords before the data is compressed.
• test_compressed_header_double_extname - included some lines checking state of internal/private properties. Those checks have been removed as the test is no longer relevant for this API.

Outstanding issues/open questions

• [ ] Add back support for accessing CompImageHDU.shape (should be simple)
• [ ] Make sure that we don't write out anything when in mode='update' if a file is simply opened and closed (test_open_comp_image_in_update_mode failure)
• [ ] Not critical for this PR, but currently this preserves the CompImageHeader class - as noted higher up we could potentially consider removing or simplifying this in future.
• [ ] This deprecates but does not yet remove the compressed_data property on CompImageHDU. We could keep it but it is probably better to encourage people to open the file with disable_image_compression? I doubt this property is used much.

[github-actions[bot]]

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

• [ ] Do the proposed changes actually accomplish desired goals?
• [ ] Do the proposed changes follow the Astropy coding guidelines?
• [ ] Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• [ ] Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• [ ] Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
• [ ] Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• [ ] Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• [ ] Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• [ ] Is a milestone set? Milestone must be set but we cannot check for it on Actions; do not let the green checkmark fool you.
• [ ] At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

[github-actions[bot]]

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

[astrofrog]

Closing as the astropy PR is now open.