wcs.naxis should be determined from NAXIS, not presence of C???? keywords by mdboom · Pull Request #323 · astropy/astropy

mdboom · 2012-08-02T13:47:11Z

If I parse the following header:

SIMPLE  =                    T / conforms to FITS standard
BITPIX  =                  -64 / array data type
NAXIS   =                    2 / number of array dimensions
NAXIS1  =                  600
NAXIS2  =                  600
CTYPE1  = 'RA---SIN'
CRVAL1  =   1.000000000000E+02
CDELT1  =  -1.000000000000E-04
CRPIX1  =   3.010000000000E+02
CUNIT1  = 'deg     '
CTYPE2  = 'DEC--SIN'
CRVAL2  =  -2.000000000000E+00
CDELT2  =   1.000000000000E-04
CRPIX2  =   3.010000000000E+02
CUNIT2  = 'deg     '
CTYPE3  = 'VRAD    '
CRVAL3  =  -1.000000000000E+04
CDELT3  =   1.000000000000E+04
CRPIX3  =   1.000000000000E+00
CUNIT3  = 'm/s     '
CTYPE4  = 'STOKES  '
CRVAL4  =   1.000000000000E+00
CDELT4  =   1.000000000000E+00
CRPIX4  =   1.000000000000E+00
CUNIT4  = '        '
END

with:

In [1]: from astropy.io import fits

In [2]: header = fits.Header.fromtextfile('header.hdr')

In [3]: from astropy import wcs

In [4]: w = wcs.WCS(header)

the naxis attribute has the wrong value:

In [5]: print w.naxis
4

as I would imagine NAXIS takes precedence over anything else. If I try and access naxis3 for example, I have issues:

In [6]: print w.naxis3
ERROR: AttributeError: 'WCS' object has no attribute 'naxis3' [IPython.core.interactiveshell]
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
/Volumes/Raptor/Dropbox/code/development/APLpy/APLpy/<ipython-input-6-8c58730432b6> in <module>()
----> 1 print w.naxis3

AttributeError: 'WCS' object has no attribute 'naxis3'

as expected.

embray · 2012-07-27T19:45:51Z

Well it's not Header.fromtextfile that's the problem. So I don't know...

mdboom · 2012-07-29T21:43:15Z

This behavior is inherited from wcslib. See here:

https://github.com/astropy/astropy/blob/master/astropy/wcs/src/wcslib/C/wcshdr.h#L381

Admittedly, we should probably copy these notes to the Python documentation -- this issue has come up before for pywcs -- it's a little confusing but apparently how the WCS standard is supposed to work.

mdboom · 2012-07-29T21:45:24Z

If I can paraphrase the reason I was told for this behavior:

It's possible that a WCS may want to specify extra dimensions while still having fewer dimensions in the actual data itself (which of course uses NAXIS).

Also, it is possible to limit the number of dimensions by passing naxis=2 to the constructor.

astrofrog · 2012-07-30T10:22:29Z

But for those kinds of datasets, why not have NAXIS=4 and set NAXIS3 and NAXIS4 to 1?

I guess what confused me was that regardless of this behavior, I would have thought wcs.naxis should be equal to NAXIS, and I would also expect the default behavior to be to accept NAXIS values for transformations. In the example where I ran into an issue, the 3rd and 4th dimensions were just residuals from a slicing algorithm which reduced NAXIS but forgot to remove the associated WCS keywords.

Wouldn't a more sensible behavior be to respect NAXIS by default, and allow the user to override it with WCS(..., naxis=...) which would cause the missing NAXIS* keywords to be set to 1 and the further WCS keywords to be read (with an exception being raised if they are missing). This would seem more natural to me. If further keywords are present compared to NAXIS, one could even raise an INFO message to let the user know they have the option to force naxis to a higher value.

mdboom · 2012-07-31T14:09:40Z

There is justification for this behavior in Paper I, section 2.2.

http://www.atnf.csiro.au/people/mcalabre/WCS/wcs.pdf

(I suggest reading that section in its entirety, but it essentially amounts to "the addition of degenerate axes to otherwise n-dimensional data may mean some software will reject it unnecessarily").

None of this precludes what you're suggesting, but I'm wary to deviate from the behavior of wcslib unless there's something that can't be done any other way. In this particular case, one can pass an explicit naxis kwarg to the constructor to force a requested number of axes. However, there are some things we can do to make this easier on the user. I propose:

Adding a warning when NAXIS < naxis
Changing wcs.naxis to be equal to the NAXIS FITS keyword is a bad idea. It should match the number of axes in the transformation, not in the data. And this is a class for WCS transformations, after all. We have io.fits/pyfits to handle the dimensions of the data.
naxis1 and naxis2 should probably be made private (through some gradual deprecation perhaps, since that's backward-incompatible). They are used pretty much exclusively by the footprint calculation which is 2D only. Having them available here just causes confusion, and they really should be obtained directly from the dimensions of the data array in the pyfits HDU object. I could also be persuaded here that we define naxisX for all X <= wcs.naxis, and the missing values are 1. That would at least provide a consistent interface to get dimensions even when naxis > NAXIS and wouldn't force the use of degenerate axes in the FITS file itself.

EDIT: Changed != to < in proposal #1.

… determined is somewhat confusing.

…4adfc14

mdboom · 2012-08-02T13:47:46Z

I've put my proposals into code here so you can play with it.

astrofrog · 2012-08-13T09:09:46Z

Sorry for the delay, I will try this out today.

astrofrog · 2012-08-13T09:10:15Z

Are the changes in this file needed?

Yes. This header was actually only working by "accident" -- the wcslib parser is fuzzy about FITS headers with carriage returns. It doesn't complain -- it reads some cards that don't overlap an 80 character boundary and silently rejects others. I didn't notice this problem with this header until, in this PR, the header needed to be re-parsed by io.fits in order to read the NAXIS card.

astrofrog · 2012-08-13T11:46:11Z

I like the addition of the warning and that naxis keywords default to 1. Just a couple of comments:

why the changes in nonstandard_units.hdr?
I also do understand now why naxis? should not really be public attributes, in the sense that the WCS transformation doesn't depend on that. In fact, one could even remove it completely by making the footprint calculator ask for a given header or dimensions (e.g. I might want to calculate the footprint for different sized images for the same WCS transformation). In any case, I'd be fine with having the naxis? attributes be deprecated if that's what you think is best.

mdboom · 2012-08-14T04:55:48Z

I have updated this to WCS.naxis1 and WCS.naxis2 are deprecated, negating the need to set naxisN to 1 etc.

@iguananaut: I added a deprecate_attribute function that piggy backs on the deprecate decorator. It's of course possible to do this by turning the attribute into a property and deprecating the property's methods, but this turns all of that into a one liner in the case where the property doesn't need to do anything special. You might want to have a look as the original @deprecate author.

astrofrog · 2012-08-14T10:11:23Z

Ok, this looks good and consistent to me, so feel free to merge!

wcs.naxis should be determined from NAXIS, not presence of C???? keywords

astrofrog · 2012-08-18T16:18:28Z

It looks like this caused some failures with Python 3:

https://jenkins.shiningpanda-ci.com/astropy/job/astropy-master-osx-10.7-multiconfig/145/

mdboom · 2012-08-18T17:25:02Z

Thanks. I think this is fixed in master now.

… determined is somewhat confusing.

wcs.naxis should be determined from NAXIS, not presence of C???? keywords

Updating bundled sphinx-automodapi to v0.6

ghost assigned mdboom Jul 26, 2012

mdboom added 2 commits August 2, 2012 09:33

Possible solution for astropy#323, that the way in which wcs.naxis is…

4adfc14

… determined is somewhat confusing.

Fix from problems with header <-> string conversion revealed by commit …

0643ccd

…4adfc14

Remove some accidental debugging statements

da0dda0

astrofrog reviewed Aug 13, 2012
View reviewed changes

mdboom added 2 commits August 13, 2012 10:10

Fix typo in docstring

1c1f910

Deprecate naxis1, naxis2

4217199

mdboom added a commit that referenced this pull request Aug 18, 2012

Merge pull request #323 from mdboom/wcs/naxis

e12c914

wcs.naxis should be determined from NAXIS, not presence of C???? keywords

mdboom merged commit e12c914 into astropy:master Aug 18, 2012

mdboom mentioned this pull request Feb 25, 2013

WCS objects should have a naxis3 attribute if appropriate #816

Closed

keflavich pushed a commit to keflavich/astropy that referenced this pull request Oct 9, 2013

Possible solution for astropy#323, that the way in which wcs.naxis is…

39a82b6

… determined is somewhat confusing.

keflavich pushed a commit to keflavich/astropy that referenced this pull request Oct 9, 2013

Merge pull request astropy#323 from mdboom/wcs/naxis

8752893

wcs.naxis should be determined from NAXIS, not presence of C???? keywords

mdboom deleted the wcs/naxis branch May 22, 2014 00:00

astrofrog mentioned this pull request Mar 5, 2016

astropy.wcs: access to NAXIS1 and NAXIS2 #4669

Closed

astrofrog added a commit to astrofrog/astropy that referenced this pull request Jun 12, 2019

Merge pull request astropy#323 from astrofrog/automodapi-v0.6

15915ec

Updating bundled sphinx-automodapi to v0.6

richeshgupta mentioned this pull request Sep 23, 2026

SWR astropy__astropy-323 razorpaytest/astropy#75

Open

Conversation

mdboom commented Aug 2, 2012

Uh oh!

embray commented Jul 27, 2012

Uh oh!

mdboom commented Jul 29, 2012

Uh oh!

mdboom commented Jul 29, 2012

Uh oh!

astrofrog commented Jul 30, 2012

Uh oh!

mdboom commented Jul 31, 2012

Uh oh!

mdboom commented Aug 2, 2012

Uh oh!

astrofrog commented Aug 13, 2012

Uh oh!

astrofrog Aug 13, 2012

Choose a reason for hiding this comment

Uh oh!

mdboom Aug 14, 2012

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Aug 13, 2012

Uh oh!

mdboom commented Aug 14, 2012

Uh oh!

astrofrog commented Aug 14, 2012

Uh oh!

astrofrog commented Aug 18, 2012

Uh oh!

mdboom commented Aug 18, 2012

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants