ejeschke / ginga

The Ginga astronomical FITS file viewer
BSD 3-Clause "New" or "Revised" License
122 stars 77 forks source link

Primary header should be inherited #93

Closed pllim closed 9 years ago

pllim commented 9 years ago

Currently, in an exposure with [PRIMARY, SCI, ERR, DQ] extensions, I have to explicitly use MultiDim to access the primary header. A more intuitive behaviour for this format is to have Header of [SCI, ERR, DQ] to automatically inherit PRIMARY header, and not even display PRIMARY as an option in MultiDim. This format is widely used for HST (and possibly JWST).

I have an idea on how to do this by inheriting your Header class and then load the PRIMARY with brute force. Is there a more elegant way to do this?

ejeschke commented 9 years ago

Hi @pllim, my thinking is that the best place to do this currently is by modifying MultiDim. Because it is simply creating new AstroImage objects and inserting them into the viewer, it can simply grab the main header instead of the HDU header (take a look at the code in the set_hdu method).

This then becomes a simple setting (preference) that we would create for MultiDim, similar to the preferences that there are for other plugins. I think this is a few line mod at most. Take a look at MultiDim and tell me what you think. If you don't want to submit a PR I can probably get to this in the next few days.

pllim commented 9 years ago

Not really. I think io_fits.py is a better place for this.

pllim commented 9 years ago

Anyways, I'll submit a PR later today and we can discuss. :)

ejeschke commented 9 years ago

I'm testing this PR now. Is it merging the primary header with the individual HDU headers? The header seems to be changing as I scroll through the HDUs in MultiDim. So it is not just displaying the primary header, but merging it with the HDU header, with the HDU header keywords having preference?

pllim commented 9 years ago

Yes, but let me explain. My idea is to display the header such that the original header is displayed first, followed by inherited primary header. If there are conflicting keywords, the ones in primary are ignored. As you can see, I merge them in AstroImage.get_header() method without changing AstroImage.metadata['header']. So theoretically, I can still extract only the original header by calling image.metadata['header'], right? My idea is also to make AstroImage.inherit_primary_header configurable using your built-in config system, but I am not very sure how to do that elegantly.

ejeschke commented 9 years ago

I see that some HST images have a FITS keyword INHERIT=T in the HDU headers. Should we make this behavior only happen if that keyword exists and its value is true?

pllim commented 9 years ago

I don't think INHERIT is a normal FITS standard (maybe @embray can confirm). According to http://www.stsci.edu/hst/HST_overview/documents/datahandbook/intro_ch23.html , "the FITS standard does not include keyword inheritance, and while the idea itself is simple, its consequences are often complex and sometimes surprising to users." Ginga should not be dragged into STScI's weird formats (too much).

embray commented 9 years ago

I agree with @pllim here; INHERIT causes more confusion than it's worth.

ejeschke commented 9 years ago

In your current PR though, inherit is set as the default. If "its consequences are often complex and sometimes surprising to users" then I'm thinking that maybe it should off by default. The FITS keyword (if standard in HST headers) would be an easy way to turn it on without having to set a preference (we could make a preference, of course).

pllim commented 9 years ago

@ejeschke , I set inherit to default because I need it for my work. My thought is that once it is absorbed into the config system, it can be disabled by default, and then I can just enable it in my config. I should have explained in the code, I apologize.

Won't things get even more confusing if user can choose to turn on inheritance either by using config or INHERIT keyword? What if they do not agree, then which one takes precedence?

ejeschke commented 9 years ago

I see. I don't know the FITS history of INHERIT, but it seems like if it is in the "FITS standard" then ginga should try to do the "right thing" by default. Maybe an acceptable solution would be to follow the preference if the user has set one and otherwise to check for the INHERIT keyword and follow that (if present and True), else assume inherit=False. Sound ok?

ejeschke commented 9 years ago

If this sounds ok I'll add in the preference bits and merge it.

embray commented 9 years ago

It's not in the FITS standard. It's just an HSTism, like NEXTEND (shudder).

ejeschke commented 9 years ago

Hmm, thanks for the explanation @embray. In that case, I'll just add the preference bits and set the default to be False.

pllim commented 9 years ago

:+1: @ejeschke

ejeschke commented 9 years ago

@pllim, can you test commit b4b1876ec5ad3aaace9ba62cf48666ddde67faa7 ? As suggested in this thread, this "inherit" feature is activated by preference only. Please add to your $HOME/.ginga/general.cfg the line:

# inherit keywords from the primary header when loading HDUs
inherit_primary_header = True
pllim commented 9 years ago

This works great. It also preserves the original header internally when I write out the modified buffer. Thank you!