Refactor handling of inherit_primary_header in AstroImage

ejeschke / ginga

The Ginga astronomical FITS file viewer

BSD 3-Clause "New" or "Revised" License

121 stars 77 forks source link

Refactor handling of inherit_primary_header in AstroImage #978

Closed ejeschke closed 2 years ago

ejeschke commented 2 years ago

This PR refactors AstroImage, AstroTable and ginga.util.io_fits in the following ways:

the primary header is saved as a metadata item called 'primary_header' (the header is already saved similarly as 'header'); this makes the handling of these two entities similar.
the astropy/fitsio specific details for loading an HDU has been moved from AstroImage and AstroTable to ginga.util.io_fits. This encapsulates the details of the specific support package in io_fits
added loading of FITS tables via the fitsio package in io_fits
modified the TableView class to be able to view tables loaded with either astropy or fitsio

ejeschke commented 2 years ago

@pllim, I certainly don't want to break things for stginga. I'm just trying to clean up the "include primary header" implementation because it felt inconsistent with the way the regular headers are stored and handled. Could you let me know what breaks with this PR? I'm am happy to modify it.

With this PR, the primary header is always saved, and is available from the image by image.get('primary_header'). The primary header is visible in the Header plugin by clicking the checkbox.

pllim commented 2 years ago

I'll have to double check. At the very least, there is a setting in general.cfg. I will also have to see if I call that affected API directly or not. I'll get back to you in a week or two. Thanks for your patience!

https://github.com/ejeschke/ginga/blob/04973bfe4e74d4c804abea7124d309b720b0a7a0/ginga/examples/configs/general.cfg#L51-L52

ejeschke commented 2 years ago

Thanks for your patience!

No hurry on this one, @pllim!

ejeschke commented 2 years ago

Hi @pllim, can you have another look at this? I have restored the non-private attributes and API of AstroImage and their former behavior. I will submit a separate PR later for the deprecation of the items and we can discuss the issue further in that PR.

It is true that inherit_primary_header now defaults to True in Control.py, but that is a good thing, because in the previous version it did not matter at all whether it was True or False, the primary header was always saved.

ejeschke commented 2 years ago

Rebased

ejeschke commented 2 years ago

Still needs a change log.

I don't want to be a blocker of this getting merged. But I would want to have some time to test the release candidate with this change. Thanks for your patience!

No problem, @pllim.

ejeschke commented 2 years ago

I will add some comments to the WhatsNew.

ejeschke commented 2 years ago

Rebased on master.

ejeschke commented 2 years ago

Can't keep this open forever... Feel free to merge. Thanks! smile_cat

Thanks for the great review, @pllim. If you notice anything amiss we can fix it before the next release.