ejeschke / ginga

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

Setting inherit_primary_header=True in general.cfg does not work anymore #988

Closed pllim closed 2 years ago

pllim commented 2 years ago

I have inherit_primary_header = True set in ~/.ginga/general.cfg but it no longer makes AstroImage inherit primary header on load, even though the primary header still displays in Header plugin when you ask for it. What I mean is the following:

from ginga.AstroImage import AstroImage

im = AstroImage()
im.load_file("myimage.fits")
im.get_header()  # No longer has primary header included

This causes a plugin in stginga to misbehave.

ejeschke commented 2 years ago

@pllim, I will commit an update to fix the issue, but for now, to patch your plugin against future deprecation, can you do this instead (in the plugin):

im = AstroImage()
im.load_file("myimage.fits", save_primary_header=True)
im.get_header(include_primary_header=True)
ejeschke commented 2 years ago

Sorry, last line should say include_primary_header=True, comment above updated.

ejeschke commented 2 years ago

@pllim, when I execute your code above in a version of ginga before #985, I don't get the primary header included even if inherit_primary_header=True in my general.cfg. Are you sure these are the exact lines of code getting executed in the plugin? Or do you have a modification in stginga that always forces the primary header to be included?

ejeschke commented 2 years ago

Just pushed a change that allows the following as default case:

im = AstroImage()
im.load_file("myimage.fits")
im.get_header(include_primary_header=True)

or

im = AstroImage(inherit_primary_header=True)  # to be deprecated--don't recommend
im.load_file("myimage.fits")
im.get_header()
pllim commented 2 years ago

I think we used to pick up the setting from general.cfg.

pllim commented 2 years ago

But if it has been broken for a while, then I am not sure when it started to break. Pretty sure it used to work. 🤷

ejeschke commented 2 years ago

It doesn't work in release 3.0 or 3.1. At least from an ipython session.

ejeschke commented 2 years ago

I think we used to pick up the setting from general.cfg.

The setting is picked up if the image is loaded via the machinery in Control.py, but not if you just create an AstroImage as shown above in your example. That's why it works for the Header plugin.

pllim commented 2 years ago

So, I downgraded Ginga to 3.2.0 and SNRCalc plugin works with dev version of stginga, in that the "Min SBR" field is updated properly based on INSTRUME value that is grabbed from the primary header. So definitely something in dev Ginga broke it, but I'll have to track down how exactly.

pllim commented 2 years ago

So, I think this is what happened. The plugin is a local plugin. I was accessing self.fitsimage.get_image().get_header(), where self is the local plugin. This grabbed the primary header (presumable from the general.cfg preference) in Ginga 3.2 but not the dev version when I tested it. Does this ring any bell to you?

ejeschke commented 2 years ago

self.fitsimage.get_image().get_header()

Yeah, so that would be inheriting the flag because the Control module does that. I think I see the problem now, and it is not in AstroImage. I'll work on another commit.

ejeschke commented 2 years ago

BTW, if you simply change:

self.fitsimage.get_image().get_header()

to:

self.fitsimage.get_image().get_header(include_primary_header=True)

Does the plugin work?

pllim commented 2 years ago

Yes, if I set .get_header(include_primary_header=True), it does work. However, not setting it like that but in general.cfg resulting in the setting not being respected is still a breaking change and a very subtle one at that.

ejeschke commented 2 years ago

Well, I think I may have arrived at a solution that will work for us. Try the latest commit with your unmodified plugin and let me know how it goes. I will probably still need to modify the change log a bit.

pllim commented 2 years ago

Yes, seems to work again with your commit to master with existing unmodified plugin. Thanks for the quick fix! I'll test the dev version a bit more with other things and report back. 🙇‍♀️

ejeschke commented 2 years ago

I will probably still need to modify the change log a bit.

I updated the WhatsNew changelog to match the changes.

pllim commented 2 years ago

OK as far as I am concerned, this issue is gone. I'll let you know soon about the release candidate. Sorry for the delay!