Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HDUList Cannot Handle EXTVER #6453

Closed
dborncamp opened this issue Aug 18, 2017 · 17 comments
Closed

HDUList Cannot Handle EXTVER #6453

dborncamp opened this issue Aug 18, 2017 · 17 comments
Labels

Comments

@dborncamp
Copy link

I am trying to create a multi extension .fits file that has extensions with the same name. I expected that the EXTVER would be populated when the HDUList is made because I have no way of making the version of the ImageHDU that I can see in http://docs.astropy.org/en/stable/io/fits/api/images.html#images . Here is an example of my issue in Python 2.7 using astopy 1.3.3:

from astropy.io import fits
import numpy as np

def testissue():
    # Make a primary header
    prihdr = fits.PrimaryHDU()

    # Make 2 extensions, both named 'SCI'
    sci1 = fits.ImageHDU(data=np.ones((100,100)), name='SCI')
    sci2 = fits.ImageHDU(data=np.ones((100,100)), name='SCI')

    # Make and HDULIST with all of the extensions I want
    hdulist = fits.HDUList([prihdr, sci1, sci2])

    # Write that new fits file
    hdulist.writeto('testIssue.fits', overwrite=True)

    # Now try to use extver to get information per example at
    # http://docs.astropy.org/en/stable/io/fits/
    # >>> getheader('in.fits', ('sci', 2))  # use a tuple to do the same
    fits.getheader('testIssue.fits', ('SCI', 2))

Gives:

---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
<ipython-input-140-3d136e50da12> in <module>()
     15 # http://docs.astropy.org/en/stable/io/fits/
     16 # >>> getheader('in.fits', ('sci', 2))  # use a tuple to do the same
---> 17 fits.getheader('testIssue.fits', ('SCI', 2))

/Users/dborncamp/miniconda2/envs/astro/lib/python2.7/site-packages/astropy/io/fits/convenience.pyc in getheader(filename, *args, **kwargs)
    108     hdulist, extidx = _getext(filename, mode, *args, **kwargs)
    109     try:
--> 110         hdu = hdulist[extidx]
    111         header = hdu.header
    112     finally:

/Users/dborncamp/miniconda2/envs/astro/lib/python2.7/site-packages/astropy/io/fits/hdu/hdulist.pyc in __getitem__(self, key)
    312         # instead
    313         return self._try_while_unread_hdus(super(HDUList, self).__getitem__,
--> 314                                            self._positive_index_of(key))
    315
    316     def __contains__(self, item):

/Users/dborncamp/miniconda2/envs/astro/lib/python2.7/site-packages/astropy/io/fits/hdu/hdulist.pyc in _positive_index_of(self, key)
    700         """
    701
--> 702         index = self.index_of(key)
    703
    704         if index >= 0:

/Users/dborncamp/miniconda2/envs/astro/lib/python2.7/site-packages/astropy/io/fits/hdu/hdulist.pyc in index_of(self, key)
    681
    682         if (found is None):
--> 683             raise KeyError('Extension {} not found.'.format(repr(key)))
    684         else:
    685             return found

KeyError: "Extension ('SCI', 2) not found."

Is this expected behavior? It seems a little confusing that I would need to add the extension version information after the fact.

@pllim pllim added the io.fits label Aug 18, 2017
@pllim
Copy link
Member

pllim commented Aug 18, 2017

For more info, I also see this problem in Astropy 2.0.1 with Python 3.5. The problem is that sci1 and sci2 are both registered as ('SCI', 1) in the HDU list. The workaround for now is to either:

  • set sci2.ver = 2 before creating the HDU list; OR
  • set hdulist[2].header.set('EXTVER', 2) afterwards

None of the workaround seems elegant nor intuitive.

@MSeifert04
Copy link
Contributor

Would it make sense to accept a tuple as name parameter containing the (name, ver)? It's probably the easiest way to deal with multiple extensions with the same name. Or what would be the expected behavior here?

@pllim
Copy link
Member

pllim commented Aug 18, 2017

I tried forcing name=('SCI', 2) on sci2 init but it gave me "name must be a string" error message.

@pllim
Copy link
Member

pllim commented Aug 18, 2017

Another possibility is to have an additional ver keyword, so you can do something like ImageHDU(array, name='SCI', ver=2). I don't know why it is not there. That seems the most intuitive to me.

@MSeifert04
Copy link
Contributor

MSeifert04 commented Aug 18, 2017

I tried forcing name=('SCI', 2) on sci2 init but it gave me "name must be a string" error message.

Yes, that's why I asked if that would be "useful" :)

It's much easier than trying to keep track of the names of all HDUs in a HDUList.

@dborncamp
Copy link
Author

I would expect the HDUList to add the appropriate extver based on the order they appear in the list if the exact ver is not specified.

@pllim
Copy link
Member

pllim commented Aug 18, 2017

Another thought I had was that maybe the ver used to automatically increase a long time ago but somehow stopped working? That's just a wild guess though.

@MSeifert04
Copy link
Contributor

MSeifert04 commented Aug 18, 2017

Given that the name and ver of the HDUs are mutable it's probably not a good idea for the HDUList to (try to) keep track of the names and vers. It's definitely an option but I don't think it's possible to do that while complying to the "least-astonishment" principle.

@pllim
Copy link
Member

pllim commented Aug 18, 2017

Same problem in astropy 1.2 as well, so maybe the ver was always "broken" but we never noticed because its info was not printed via .info() until #6124 .

@pllim
Copy link
Member

pllim commented Aug 18, 2017

For the record, RAW images from HST already have name and ver properly populated, though I am not sure how they did it (could very well just be an explicit header update from the Commanding team).

@MSeifert04
Copy link
Contributor

I'll see if allowing to pass in ver makes sense (and doesn't break tests). I'll report back with a PR in case I got it working.

@nden
Copy link
Contributor

nden commented Aug 19, 2017

Here's a working example:

phdu = fits.PrimaryHDU()
im1 = fits.ImageHDU(name='SCI')
im1.ver = 1
im2 = fits.ImageHDU(name='SCI')
im2.ver = 2
hdul = fits.HDUList([phdu, im1, im2])
hdul.info()
Filename: (No file associated with this HDUList)
No.    Name      Ver    Type      Cards   Dimensions   Format
  0  PRIMARY       1 PrimaryHDU       4   ()      
  1  SCI           1 ImageHDU         7   ()      
  2  SCI           2 ImageHDU         7   ()   

I've heard an explanation why ver has to be assigned and can't be passed in but I don't recall the details and I am not familiar with this code.

@MSeifert04
Copy link
Contributor

I've heard an explanation why ver has to be assigned and can't be passed in

Oh, that would be bad, would you mind also adding that as a comment in #6454? Also do you have a rough idea where you could have heard that? I searched through astropy-dev and github but I couldn't find anything (yet).

@pllim
Copy link
Member

pllim commented Aug 23, 2017

@embray ? 🙏

@saimn
Copy link
Contributor

saimn commented Aug 28, 2017

I was thinking that HDUList could handle this automatically, but after reading the standard it is allowed to have a non-unique combination of EXTNAME/EXTVER. So, #6454 is indeed a good mean to make it easier to create this kind of file, and I don't think we can do more ? (Even a warning would not really be justified with respect to the standard).

@saimn
Copy link
Contributor

saimn commented Aug 31, 2017

Closing now that #6454 was merged.

@saimn saimn closed this as completed Aug 31, 2017
@embray
Copy link
Member

embray commented Aug 20, 2018

Sorry I keep missing a lot of these pings. I'd like to start paying more attention again when I can. No promises though. But if there's ever anything that definitely needs my help (and this one clearly didn't) don't hesitate to e-mail me either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants