Handle non-ASCII characters in FITS header by saimn · Pull Request #6084 · astropy/astropy

saimn · 2017-05-16T21:01:53Z

This just replace the non-ASCII characters by the U+FFFD REPLACEMENT CHARACTER (�) and emit a warning. I wonder if it is useful to do more: with this change, the header can be loaded and printed, which will emit verification errors which allow to easily spot the issue. Then it's up to the user to fix the problematic cards, and writing will crash if the cards are not fixed.

For instance, with the broken file from #2887 and this branch:

In [1]: %astropy
Numpy 1.12.1
Astropy 2.0.dev18346

In [2]: h = fits.getheader('broken_wcs.fits')
WARNING: non-ASCII characters are present in the FITS file header and have been replaced by the U+FFFD REPLACEMENT CHARACTER [astropy.io.fits.py3compat]

In [3]: h
Out[3]: WARNING: VerifyWarning: Verification reported errors: [astropy.io.fits.verify]
WARNING: VerifyWarning: Unfixable error: Unprintable string 'Subversion date 2013-04-16 22:17:10 -0300 (��t, 16 dub'; commentary cards may only contain printable ASCII characters [astropy.io.fits.verify]
WARNING: VerifyWarning: Note: astropy.io.fits uses zero-based indexing.
 [astropy.io.fits.verify]

[snip]  
HISTORY Created by the Astrometry.net suite.                                    
HISTORY For more details, see http://astrometry.net .                           
HISTORY Subversion URL                                                          
HISTORY   http://track.astrometry.net/svn/trunk/src/astrometry/util/            
HISTORY Subversion revision 22592                                               
HISTORY Subversion date 2013-04-16 22:17:10 -0300 (��t, 16 dub                  
HISTORY   2013)                                                                 
HISTORY This WCS header was created by the program "blind".                     
[snip]  

In [5]: fits.PrimaryHDU(header=h).writeto('test.fits')
---------------------------------------------------------------------------
[snip]  

/home/simon/dev/astropy/astropy/io/fits/hdu/hdulist.py in writeto(self, fileobj, output_verify, overwrite, checksum)
    853             return
    854 
--> 855         self.verify(option=output_verify)
    856 
    857         # make sure the EXTEND keyword is there if there is extension

/home/simon/dev/astropy/astropy/io/fits/verify.py in verify(self, option)
    119                     warnings.warn(line, VerifyWarning)
    120             else:
--> 121                 raise VerifyError('\n' + '\n'.join(messages))
    122 
    123 

VerifyError: 
Verification reported errors:
HDU 0:
    Card 173:
        Unprintable string 'Subversion date 2013-04-16 22:17:10 -0300 (��t, 16 dub'; commentary cards may only contain printable ASCII characters
Note: astropy.io.fits uses zero-based indexing.

It's also possible to do more, maybe adding an option to replace these characters with a question mark, which would allow to write the fixed file without manual intervention. Or maybe this could be handled by the verification system. Thoughts ?

TODO: extract a broken header to add a unit test.

Fixes astropy#6074 and others. The non-ASCII characters are replaced by the U+FFFD REPLACEMENT CHARACTER, which will then emit verification errors but also allow to fix the problematic cards.

saimn · 2017-05-16T21:06:03Z

@pllim - And changelog is already here, yay ! (and with the correct PR number 😄 )

saimn · 2017-05-16T21:09:27Z

Putting milestone to 1.0.13 as it's a long standing issue, and it should be backportable.

MSeifert04 · 2017-05-16T21:57:37Z

wow, that would be really nice to have! 🎉

It's a shame it has to do the encoding twice but it seems there's no "replace+warn" option in decode.

saimn · 2017-05-17T08:12:52Z

So this address the non-ASCII case, but there is also another issue (#5396) which is slightly different, the "FITS header values must contain standard printable ASCII characters" error (typically "\n" in the value).

pllim · 2017-05-17T17:25:49Z

Failure seems related but since you labeled this as WIP, maybe you expected it already:

>           assert h['COMMENT'] ==  'h�llo'
E           AssertionError: assert h�llo == 'h\xef\xbf\xbdllo'

FWIW I am okay with replacing with "?" but will that violate some sort of FITS standards if this character occurs in a non-comment field (e.g., DATE-OBS or something...)? Also it is okay if #5396 fix is not included in this PR, as we don't want to get too ambitious...

saimn · 2017-05-17T19:08:20Z

@pllim - Yes I realized after that I didn't test with Python 2, I'm looking into it.
For the replacement with "?", it is a valid character I think, so it would allow to automatically fix the header. I see some weird things with � on Python 2, so maybe I will go with "?" instead.

saimn · 2017-05-17T22:18:19Z

Ok, now that I have a better understanding of the difference between py2 and py3, it seems difficult to get the same behavior. For py2 the header string is not decoded, and this works because py2 works with bytes (str). For py3 the header string is decoded to ascii because the native type is now unicode. This is where it can break, and also where we replace non-ASCII characters.
So I changed the test to py3 only, and replaced the invalid characters with "?" as discussed above.

pllim · 2017-05-17T23:20:58Z

That's fine. We're removing Python 2 in v3 anyway, so that's okay by me.

saimn · 2017-05-18T08:17:11Z

Ok, so I tested with the broken_wcs.fits file from #2887 and the 3 files from #5093, and it works. There is an issue however on Python 2 with the fitsheader script, which crashes with broken_wcs.fits, and I don't understand why.

saimn · 2017-05-18T08:33:22Z

And the issue with fitsheader and Python 2 was due to the unicode_literals import 😢
Should be good now ! (fitsheader is not tested yet, I could have skipped the CI)

pllim · 2017-05-18T13:40:54Z

 documentation.
 """
-from __future__ import (absolute_import, division, print_function,
-                        unicode_literals)


Is it okay to simply remove unicode_literals? It is used everywhere. There is even a special test marker to test both with and without unicode_literals in some cases (like table).

It applies only to the file in which it is imported, so it's possible to have modules where it is not used. And most of the time it's useless, or it causes more issues than it solves. So here I think it's safe to remove it, and it fixes the uses of fitsheader with an invalid file.

pllim

LGTM

pllim · 2017-05-19T19:13:38Z

Begone, invalid ASCII!

Handle non-ASCII characters in FITS header

Handle non-ASCII characters in FITS header

2d07272

Fixes astropy#6074 and others. The non-ASCII characters are replaced by the U+FFFD REPLACEMENT CHARACTER, which will then emit verification errors but also allow to fix the problematic cards.

saimn added the io.fits label May 16, 2017

saimn added this to the v1.0.13 milestone May 16, 2017

saimn added the Work in progress label May 16, 2017

Add test with a header containing non-ASCII characters

7581078

saimn force-pushed the fits-non-ascii-header branch from 2fcdf8f to 7581078 Compare May 17, 2017 08:18

Replace with "?" and skip py2

f12d779

saimn added Python3 Ready-for-final-review and removed Work in progress labels May 18, 2017

saimn removed the Python3 label May 18, 2017

Fix the fitsheader script for py2 (another unicode_literals bug)

37facbf

pllim reviewed May 18, 2017

View reviewed changes

pllim requested a review from MSeifert04 May 18, 2017 14:47

pllim approved these changes May 18, 2017

View reviewed changes

pllim merged commit 2d857b7 into astropy:master May 19, 2017

saimn deleted the fits-non-ascii-header branch May 19, 2017 20:22

This was referenced May 19, 2017

OSError: Empty or corrupt FITS file (HEADER) #5732

Closed

ValueError FITS header values must contain standard printable ASCII characters. Unicode in FITS header entry. #3497

Closed

saimn mentioned this pull request May 19, 2017

Different behavior of io.fits in Python 2.7 and 3.5 #5093

Closed

bsipocz pushed a commit that referenced this pull request May 23, 2017

Merge pull request #6084 from saimn/fits-non-ascii-header

0439050

Handle non-ASCII characters in FITS header

bsipocz pushed a commit that referenced this pull request May 23, 2017

Merge pull request #6084 from saimn/fits-non-ascii-header

bba5230

Handle non-ASCII characters in FITS header

bsipocz mentioned this pull request Jun 19, 2019

Workaround to deal with invalid headers? #5396

Closed

Conversation

saimn commented May 16, 2017

Uh oh!

saimn commented May 16, 2017

Uh oh!

saimn commented May 16, 2017

Uh oh!

MSeifert04 commented May 16, 2017

Uh oh!

saimn commented May 17, 2017

Uh oh!

pllim commented May 17, 2017

Uh oh!

saimn commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saimn commented May 17, 2017

Uh oh!

pllim commented May 17, 2017

Uh oh!

saimn commented May 18, 2017

Uh oh!

saimn commented May 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim May 18, 2017

Choose a reason for hiding this comment

Uh oh!

saimn May 18, 2017

Choose a reason for hiding this comment

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

pllim commented May 19, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

saimn commented May 17, 2017 •

edited

Loading

saimn commented May 18, 2017 •

edited

Loading