spacetelescope / drizzlepac

AstroDrizzle for HST images.
https://drizzlepac.readthedocs.io
BSD 3-Clause "New" or "Revised" License
50 stars 39 forks source link

error identifying primary extension number in sci.fits files #590

Closed srodney closed 3 years ago

srodney commented 4 years ago

Line 445 in catalogs.py has a check on the extname tuple, designed to catch the case where the wcs object has an empty string for the extension name, and None for the extension number: self.wcs.extname=('',None). I've been tripping on a bug that manifests when the wcs object has self.wcs.extname=('',1). The fits file that provided the WCS in this case has the WCS info (and data array) in the primary HDU, and the primary HDU has extver=1. In my case, this fits file is a drz_sci.fits file (i.e., the output of AstroDrizzle using build=False). I think it is probably a general enough case that it warrants adding an extra check to catch and correct.

https://github.com/spacetelescope/drizzlepac/blob/0fe80f657f3830234a5b232d07f5b864cc17f67b/drizzlepac/catalogs.py#L445

mcara commented 4 years ago

I've been tripping on a bug that manifests when the wcs object has self.wcs.extname=('',1)

I am curious, how can this be? I do not believe our software should be creating this kind of files. This just seems wrong from the FITS standard point of view:

EXTVER keyword. The value field shall contain an integer to be used to distinguish among different extensions in a FITS file with the same type and name, i.e., the same values for XTENSION and EXTNAME.

In my reading of the above, an EXTNAME is required for the EXTVER to make sense and to be present.

I think the real bug is in https://github.com/spacetelescope/stwcs/blob/b7c7aaa96c9c3c9dd8c9e93dbdbf90996dfe52c8/stwcs/wcsutil/hstwcs.py#L156-L157 that allowed this to happen/be supported.

If your primary header does not have EXTNAME at all but it does have EXTVER=1, does

w = stwcs.wcsutils.HSTWCS('filename.fits', ext=('', 1))

even work? Does it load the correct WCS?

Also, if using astropy.io.fits, can it find extension ('', 1) for such a file:

from astropy.io import fits
h = fits.open('filename.fits')
h[('', 1)].header

I doubt it works. I do not think we should be supporting non-standard FITS files.

CC: @stsci-hack

srodney commented 4 years ago

Well, I don't disagree. To answer your questions @mcara : This does work:

w = stwcs.wcsutils.HSTWCS('filename.fits', ext=('', 1))

But this does not:

from astropy.io import fits
h = fits.open('filename.fits')
h[('', 1)].header

I've posted a demonstration notebook here: https://github.com/srodney/drizzlepac/blob/drizzlepac_wcs_test/tests/drizzlepac_wcs_test.ipynb

Would a workable solution be to simply change https://github.com/spacetelescope/stwcs/blob/b7c7aaa96c9c3c9dd8c9e93dbdbf90996dfe52c8/stwcs/wcsutil/hstwcs.py#L156

to

extname = ehdr.get('EXTNAME', None)

?

Another option, as illustrated at the bottom of that demo notebook, would be to fix AstroDrizzle so it sets extname='' instead of extname=None in cases like this. I haven't identified where that change would need to go.

mcara commented 4 years ago

This does work:

w = stwcs.wcsutils.HSTWCS('filename.fits', ext=('', 1))

I do not think so. Just because it does not crash right away, it does not mean it loads the correct WCS. Please try printing w. You will see that it is most likely not the WCS you are expecting to see.

Why not simply delete 'EXTVER' from the primary header if 'EXTNAME' is not present in your files? Is this not an option (not possible)?

stsci-hack commented 3 years ago

This error only arose due to astrodrizzle NOT correctly updating the headers of the products it write out in the first place. These problems have now been resolved in #954.

Granted, it would be useful for STWCS to have logic to avoid this problem as well, but #954 will insure that all astrodrizzle products will be consistent and correct without needing that logic in STWCS.

mcara commented 3 years ago

This error only arose due to astrodrizzle NOT correctly updating the headers of the products it write out in the first place. These problems have now been resolved in #954.

The thing is I still have not heard back about how EXTVER without EXTNAME came to happen in those files. That is a bug. Not having these keywords in the primary header is the right thing to do according to FITS standard.

I do not believe there is an issue with astrodrizzle at all. My opinion.