wcs problems when locale numeric separator is not a dot #313

Closed
wants to merge 2 commits into
from

Projects

None yet

5 participants

@yannick1974

When Python locale module is used, and when locale.LC_NUMERIC is set to a locale where the decimal separator is not a dot (for instance "fr_FR@UTF8"), the wcs module fails to manipulate fits headers. For instance:

from astropy.io import fits
from astropy import wcs
import locale
locale.setlocale(locale.LC_NUMERIC,'fr_FR@UTF8')
fitsFile = '/path/to/a/fits/file'
f = fits.open(fitsFile)
w = wcs.WCS(f[0].header)

This will issue a warning:

WARNING: FITSFixedWarning: 'celfix' made the change 'Singular transformation
matrix, CDELT1 is zero'. This FITS header contains non-standard content.
[astropy.wcs.wcs]

and (what's really bad) the content of WCS will be truncate to its integer part. For instance:

In [9]: w.wcs.crpix
Out[9]: array([ 1920.,  1920.])

when one would expect:

In [7]: w.wcs.crpix
Out[7]: array([ 1920.5,  1920.5])

To understand further the problem, you can do this:

from astropy.io import fits
from astropy import wcs
import locale
fitsFile = '/path/to/a/fits/file'
f = fits.open(fitsFile)
w = wcs.WCS(f[0].header)
print w.to_header()

That will show you the WCS part of your fits header. If you do a

print w.wcs

that will print the (I think) structure of the (maybe) wcslib object. Now, change the locale and print again the WCS header:

locale.setlocale(locale.LC_NUMERIC,'fr_FR@UTF8')
print w.to_header()

This will issue warnings (VerifyWarning) for astropy.io.fits.verify and the printed header will contain coma (,) instead of dots as decimal separator. This can also be seen in the content of w.wcs.

This is rather important because sometime the locale module is automatically loaded and set to your system locale, in particular when the gtk module is used, for instance when:

  • using bpython
  • using matplotlib (so when using ipython --pylab) with the GtkAgg backend

I think issue #2 of aplpy (that Thomas could not reproduce) is linked to this problem.

Thanks to Simon Conseil for his help in chasing this bug (in fact, he discovered it!). We did not go further in the analysis but we think the problem lies in the 'link' between wcs module and wcslib. We can of course help and do testing to get rid of the problem.

Regards,

Yannick

@saimn saimn referenced this pull request in aplpy/aplpy Jul 10, 2012
Closed

SingularMatrixError on each aplpy.FITSFigure call #2

@saimn
Contributor
saimn commented Jul 10, 2012

thanks Yannick for writing the bug report ;-).
Here is an example when reading the wcs of a CFHTLS image, before and after setting the locale to fr_FR@UTF8. You can see that in w.wcs dots are replaced with commas, just after changing the locale : https://gist.github.com/3082829

@mdboom
Member
mdboom commented Jul 10, 2012

Thanks for the report.

Unfortunately, I'm unable to reproduce. Can you link to a FITS file that you're using? What platform are you on? What version of Python? (I'm on Fedora 17 with Python 2.7).

I do see commas being used in the output of str(w.wcs), but I'm not concerned about that, as that output is for human consumption. The output of w.to_header() does not use commas as a decimal separator (at least for me) and that's what matters for producing valid FITS files.

@mdboom
Member
mdboom commented Jul 10, 2012

Just some notes as I do my research on this:

There doesn't seem to be a thread-safe way to temporarily turn local-dependent formatting off in C. What Numpy does is to generate the string and then go back and replace the locale's separator with '.' (see numpyos.c:_ensure_decimal_point, as part of the whole _fix_ascii processing). Python rewrote the double formatting code from the C stdlib as PyOS_double_to_string. It looks like we may have to do something similar. It would be nice to just call into either Numpy or Python's solution, however, this is something that probably needs to go in upstream wcslib, and therefore can't rely on anything Python-related.

@saimn
Contributor
saimn commented Jul 10, 2012

I'm also using Fedora 17, python 2.7, 64bits, astropy installed in a virtualenv (but I reproduce the bug also with pyfits and pywcs from the system). Here is the output of w.to_header() just after setting the locale:

In [11]: w.to_header()
Out[11]: WARNING: Output verification result: [astropy.io.fits.verify]
WARNING: VerifyWarning: Card 'CRPIX1' is not FITS standard (invalid value string: 9677,5 / Pixel coordinate of reference point).  Fixed 'CRPIX1' card to meet the FITS standard. [astropy.io.fits.verify]
WARNING: Note: PyFITS uses zero-based indexing.
 [astropy.io.fits.verify]
WARNING: VerifyWarning: Card 'CRPIX2' is not FITS standard (invalid value string: 9677,5 / Pixel coordinate of reference point).  Fixed 'CRPIX2' card to meet the FITS standard. [astropy.io.fits.verify]
WARNING: VerifyWarning: Card 'PC1_1' is not FITS standard (invalid value string: -5,166666789E-05 / Coordinate transformation matrix element).  Fixed 'PC1_1' card to meet the FITS standard. [astropy.io.fits.verify]
WARNING: VerifyWarning: Card 'PC2_2' is not FITS standard (invalid value string: 5,166666789E-05 / Coordinate transformation matrix element).  Fixed 'PC2_2' card to meet the FITS standard. [astropy.io.fits.verify]
WARNING: VerifyWarning: Card 'CRVAL1' is not FITS standard (invalid value string: 36,49583333 / [deg] Coordinate value at reference point).  Fixed 'CRVAL1' card to meet the FITS standard. [astropy.io.fits.verify]
WARNING: VerifyWarning: Card 'CRVAL2' is not FITS standard (invalid value string: -4,494444444 / [deg] Coordinate value at reference point).  Fixed 'CRVAL2' card to meet the FITS standard. [astropy.io.fits.verify]
WARNING: VerifyWarning: Card 'LATPOLE' is not FITS standard (invalid value string: -4,494444444 / [deg] Native latitude of celestial pole).  Fixed 'LATPOLE' card to meet the FITS standard. [astropy.io.fits.verify]

WCSAXES =                    2 / Number of coordinate axes                      
CRPIX1  = '9677,5  '           / Pixel coordinate of reference point            
CRPIX2  = '9677,5  '           / Pixel coordinate of reference point            
PC1_1   = '-5,166666789E-05'   / Coordinate transformation matrix element       
PC2_2   = '5,166666789E-05'    / Coordinate transformation matrix element       
CDELT1  =                    1 / [deg] Coordinate increment at reference point  
CDELT2  =                    1 / [deg] Coordinate increment at reference point  
CUNIT1  = 'deg'                / Units of coordinate increment and value        
CUNIT2  = 'deg'                / Units of coordinate increment and value        
CTYPE1  = 'RA---TAN'           / Right ascension, gnomonic projection           
CTYPE2  = 'DEC--TAN'           / Declination, gnomonic projection               
CRVAL1  = '36,49583333'        / [deg] Coordinate value at reference point      
CRVAL2  = '-4,494444444'       / [deg] Coordinate value at reference point      
LONPOLE =                  180 / [deg] Native longitude of celestial pole       
LATPOLE = '-4,494444444'       / [deg] Native latitude of celestial pole        
RESTFRQ =                    0 / [Hz] Line rest frequency                       
RESTWAV =                    0 / [Hz] Line rest wavelength                      
EQUINOX =                 2000 / [yr] Equinox of equatorial coordinates

The CFHTLS image is available here (quite big, 1.4G) : http://www.cadc-ccda.hia-iha.nrc-cnrc.gc.ca/data/pub/CFHT/CFHTLS_D-85_u_022559-042940_T0006

We can also reproduce the bug with GALEX images, for example http://galex.stsci.edu/data/GR6/pipe/01-vsn/06851-XMMLSS_00/d/01-main/0001-img/07-try/XMMLSS_00-nd-int.fits.gz, the image used for Yannick's bug report and which has a much more reasonnable size (21M gzipped). The ipython log with this image: https://gist.github.com/3083110

@yannick1974

Hi Michael,

Here I'm with de Debian Sid x86_64 with Python 2.7. In addition to what Simon posted, you can test with this lighter Herschel map:

http://hedam.oamp.fr/HerMES/data/files/DR1/images/CD_Abell-2218_image_SMAP250_dr1.fits

Note that this map won't produce warnings (because numbers are integer I suppose) but you can see the switch from point to coma in the outputs.

EDIT: Sorry, you will see the warning if you use a correct HDU e.g. 1.

@embray
Member
embray commented Jul 10, 2012

But WCS.to_header() doesn't format the FITS cards itself does it? If invalid values are getting written into the cards wouldn't that be a pyfits issue?

Except that confuses me too, because pyfits doesn't do any locale-based string formatting. Changing the locale shouldn't change the output of basic string formatting.

@mdboom
Member
mdboom commented Jul 10, 2012

@saimn, @yannick1974: Thanks for the files. I am now able to reproduce.

When you say "we think the problem lies in the 'link' between wcs module and wcslib", is there an alternative way of linking that would resolve this? As far as I know, there is a single global locale setting for the entire process, so as long as we link to wcslib (dynamically or otherwise), it will pick up whatever locale has been set for the process. Or am I missing something?

@ignaunanaut: to_header() calls wcslib's wcshdo() under the hood, which generates a string for the header in C. wcslib doesn't provide a way to get the FITS key/value pairs in any structured way, so we get that string, and then parse it with pyfits after the fact to get a pyfits Header object.

Changing the locale doesn't normally have any impact on basic string formatting in Python, however, in C it does, so what we're seeing here is the result of wcslib's C code.

I'm working on a fix now. This will probably involve patching wcslib which will hopefully be accepted upstream.

@embray
Member
embray commented Jul 10, 2012

I understand now, looking over the code. Definitely something to fix in wcslib.

@mdboom mdboom Fixes issue #313. Updates all floating point parsing/writing to use n…
…ew methods that are locale-independent and always use "." as a decimal separator.
eda9c74
@mdboom
Member
mdboom commented Jul 10, 2012

I've attached a patch that seems to resolve this issue. I've also presented it upstream to Mark Calabretta for inclusion in wcslib if possible. It's rather invasive, and unnecessary in the context of non-GUI applications (or more specifically, applications that don't set the locale), so I'm curious to see how it will be received.

@saimn
Contributor
saimn commented Jul 11, 2012

thanks !

@eteq
Member
eteq commented Jul 11, 2012

@mdboom - do you think we should merge this now if it's ready, or should we wait to hear if there's going to be an upstream fix?

@yannick1974

@mdboom I've recompiled astropy with your patch and it works very well. Thanks. Concerning the upstream inclusion / correction, it would be of course way better because distribution prefer to link against system libraries (see current discussion about packaging on the mailing list).

@mdboom
Member
mdboom commented Jul 11, 2012

@eteq: I think we should wait until there is at least buy in for an upstream fix. If this isn't going to be merged upstream, we have to deal with this problem externally in a rather different (and trickier) way.

@mdboom
Member
mdboom commented Jul 12, 2012

Just a note -- this patch has been accepted upstream and will make it into the next wcslib release. Once that's out, I plan to incorporate that new release and move everything to cextern, since we'll no longer be needing to maintain a patched wcslib. So -- hold off on merging for now. Stay tuned...

@yannick1974

That's great. Big thanks.

@mdboom mdboom referenced this pull request Jul 29, 2012
Merged

Update to wcslib 4.14 #327

@mdboom
Member
mdboom commented Jul 29, 2012

Closing in favor of #327.

@mdboom mdboom closed this Jul 29, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment