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

can not parse the fits header with more than 2 axis #66

Open
henrysting opened this issue Dec 19, 2015 · 14 comments
Open

can not parse the fits header with more than 2 axis #66

henrysting opened this issue Dec 19, 2015 · 14 comments
Assignees
Labels
Milestone

Comments

@henrysting
Copy link

It can not work with the fits file which has more than 2 axis, like in radio data.
For example, a header as below :

SIMPLE  =                    T /Standard FITS                                   
BITPIX  =                  -32 /Floating point (32 bit)                         
NAXIS   =                    4                                                  
NAXIS1  =                 1600                                                  
NAXIS2  =                 1600                                                  
NAXIS3  =                    1                                                  
NAXIS4  =                    1                                                  
EXTEND  =                    T                          
EQUINOX =   2.000000000000E+03                                                  
RADESYS = 'FK5     '                                                            
LONPOLE =   1.800000000000E+02                                                  
LATPOLE =  -3.529166667528E+00                                               
CTYPE1  = 'RA---SIN'                                                            
CRVAL1  =   4.201666666401E+01                                                  
CDELT1  =  -8.333333333333E-05                                                  
CRPIX1  =   8.010000000000E+02                                                  
CUNIT1  = 'deg     '                                                            
CTYPE2  = 'DEC--SIN'                                                            
CRVAL2  =  -3.529166667528E+00                                                  
CDELT2  =   8.333333333333E-05                                                  
CRPIX2  =   8.010000000000E+02                                                  
CUNIT2  = 'deg     '                                                            
CTYPE3  = 'STOKES  '                                                            
CRVAL3  =   1.000000000000E+00                                                  
CDELT3  =   1.000000000000E+00                                                  
CRPIX3  =   1.000000000000E+00                                                  
CUNIT3  = '        '                                                            
CTYPE4  = 'FREQ    '                                                            
CRVAL4  =   1.519499768816E+09                                                  
CDELT4  =   1.024001037953E+09                                                  
CRPIX4  =   1.000000000000E+00                                                  
CUNIT4  = 'Hz      '                                                            
PV2_1   =   0.000000000000E+00                                                  
PV2_2   =   0.000000000000E+00                                                  

It will cause the error:

  File "build/bdist.linux-x86_64/egg/pyregion/__init__.py", line 57, in as_imagecoord
  File "build/bdist.linux-x86_64/egg/pyregion/ds9_region_parser.py", line 184, in sky_to_image
  File "build/bdist.linux-x86_64/egg/pyregion/wcs_helper.py", line 231, in _get_radesys
ValueError: too many values to unpack

@mhardcastle
Copy link
Contributor

This is true, but in a sense ds9's fault: as ds9 does not put Stokes or frequency values into its regions even when applied to a cube, pyregion is necessarily going to have trouble applying them to images.

In my ds9 radio flux measurement plugin http://github.com/mhardcastle/radioflux I go to a lot of trouble to 'flatten' radio maps down to the 2D form expected to allow pyregion to apply regions to them. I'm not sure this is logic that wants to be in pyregion itself -- e.g. what should you do if you actually have a cube, as opposed to the (1,1,y,x) shape of many radio maps?

@bsipocz
Copy link
Member

bsipocz commented Jul 28, 2016

@henrysting @mhardcastle - There are plans to deprecate pyregions when the replacement package, regions, is mature enough. You may want to take a look at it, and raise feature requests there rather than here.

Regarding the original issue, maybe the spectral-cube package is more relevant place for it.

@cdeil cdeil added this to the whishlist milestone Aug 1, 2016
@cdeil cdeil added the question label Aug 1, 2016
@leejjoon
Copy link
Contributor

leejjoon commented Aug 1, 2016

I have been knowing this issue, but was not quite sure what the right way to fix is.
As a workaournd, I recommend you to read the header as a wcs object and its 'sub' method to retrieve space part of the axes. Maybe we can safely do this within the pyregion,

wcs = pywcs.WCS(f[0].header)
wcs_im = wcs.sub([1,2]) # index starts at 1
# wcs_im = wcs.sub([pywcs.WCSSUB_LONGITUDE, pywcs.WCSSUB_LATITUDE])
r2 = r.as_imagecoord(wcs_im)

@keflavich
Copy link
Contributor

keflavich commented Aug 1, 2016

wcs.sub([wcs.WCSSUB_CELESTIAL]) is probably the 'best' way to do this

EDIT: the shortcut for that is just wcs.celestial

@cdeil
Copy link
Member

cdeil commented Aug 1, 2016

Does anyone here have time to make a pull request adding this example to the docs?

@cdeil cdeil modified the milestones: v1.2, whishlist Aug 1, 2016
@mhardcastle
Copy link
Contributor

I will try to test this and document it. But it might be even better if this were the default behaviour in pyregion, since ds9 regions are always spatial, and so always need to be applied to the celestial subset of wcs?

@mhardcastle
Copy link
Contributor

Documented, pull request raised.

@cdeil
Copy link
Member

cdeil commented Aug 2, 2016

@mhardcastle documented this in #81 .

But it might be even better if this were the default behaviour in pyregion, since ds9 regions are always spatial, and so always need to be applied to the celestial subset of wcs?

I don't know. To me both options seem reasonable:

  1. letting the user call wcs.sub([wcs.WCSSUB_CELESTIAL]) before passing it to pyregion
  2. calling wcs.sub([wcs.WCSSUB_CELESTIAL]) on input in pyregion methods to automatically do this for users as a convenience

Another option would be to check the wcs on input and give a better errror message.

@leejjoon @keflavich @astrofrog - Thoughts?

@leejjoon
Copy link
Contributor

leejjoon commented Aug 2, 2016

My inclination is option 2, but only when fits header is given as an argument. I would suggest that no automatic call is made If wcs object is given. Just in case of any odd cases.

@keflavich
Copy link
Contributor

I also favor option 2. I think it should be applied even if a wcs argument is passed.

However, if you prefer not to do this for the wcs input option, we should check that wcs.naxis==2 or do a try/except for coordinate conversion, and catch the failure with a suggestion that the user use wcs.celestial instead.

@leejjoon
Copy link
Contributor

leejjoon commented Aug 2, 2016

I was just wondering if there is any corner cases where a user does not want to have the 'sub' method called. After a second thought, I think we can have 'sub' called only when the naxis is larger than 2.

@cdeil cdeil modified the milestones: 2.0, v1.2 Aug 11, 2016
sargas added a commit to sargas/pyregion that referenced this issue Sep 5, 2016
@cdeil
Copy link
Member

cdeil commented May 15, 2017

Reading through this issue and #87 and stuff linked to from there, I'm confused.

Can someone involved here please comment on status in pyregion master and is possible also whether this is now covered by a test and mentioned in the docs correctly?

@leejjoon
Copy link
Contributor

I believe the issue itself was fixed, but I don't think the test case is added (I have opened a new PR #113 with a simple test case). This also needs to be documented I believe.

@cdeil
Copy link
Member

cdeil commented May 17, 2017

@leejjoon - Can you also update the docs if needed in #113 to clear this issue out for the upcoming 2.0 release?

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