Repository navigation
FITS unit parsing with "1 / ..." sometimes fails #3986
Description
As mentioned in #602 (comment), the FITS unit parsing with "1 / .." sometimes works and sometimes fails:
>>> from astropy.units import Unit
>>> Unit('1/m', format='fits')
Unit("1 / m")
>>> Unit('1/m/s', format='fits')
Traceback (most recent call last):
File "/Users/deil/Library/Python/3.4/lib/python/site-packages/astropy-1.1.dev12994-py3.4-macosx-10.10-x86_64.egg/astropy/units/format/generic.py", line 414, in _do_parse
return self._parse_unit(s, detailed_exception=False)
File "/Users/deil/Library/Python/3.4/lib/python/site-packages/astropy-1.1.dev12994-py3.4-macosx-10.10-x86_64.egg/astropy/units/format/fits.py", line 101, in _parse_unit
cls._validate_unit(unit)
File "/Users/deil/Library/Python/3.4/lib/python/site-packages/astropy-1.1.dev12994-py3.4-macosx-10.10-x86_64.egg/astropy/units/format/fits.py", line 90, in _validate_unit
cls._to_decomposed_alternative)))
ValueError: Unit '1/m/s' not supported by the FITS standard.
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/Users/deil/Library/Python/3.4/lib/python/site-packages/astropy-1.1.dev12994-py3.4-macosx-10.10-x86_64.egg/astropy/units/format/generic.py", line 417, in _do_parse
return self._parser.parse(s, lexer=self._lexer, debug=debug)
File "/Users/deil/Library/Python/3.4/lib/python/site-packages/astropy-1.1.dev12994-py3.4-macosx-10.10-x86_64.egg/astropy/extern/ply/yacc.py", line 265, in parse
return self.parseopt_notrack(input,lexer,debug,tracking,tokenfunc)
File "/Users/deil/Library/Python/3.4/lib/python/site-packages/astropy-1.1.dev12994-py3.4-macosx-10.10-x86_64.egg/astropy/extern/ply/yacc.py", line 1047, in parseopt_notrack
tok = self.errorfunc(errtoken)
File "/Users/deil/Library/Python/3.4/lib/python/site-packages/astropy-1.1.dev12994-py3.4-macosx-10.10-x86_64.egg/astropy/units/format/generic.py", line 360, in p_error
raise ValueError()
ValueError
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/Users/deil/Library/Python/3.4/lib/python/site-packages/astropy-1.1.dev12994-py3.4-macosx-10.10-x86_64.egg/astropy/units/core.py", line 1796, in __call__
return f.parse(s)
File "/Users/deil/Library/Python/3.4/lib/python/site-packages/astropy-1.1.dev12994-py3.4-macosx-10.10-x86_64.egg/astropy/units/format/fits.py", line 147, in parse
result = super(Fits, self).parse(s, debug)
File "/Users/deil/Library/Python/3.4/lib/python/site-packages/astropy-1.1.dev12994-py3.4-macosx-10.10-x86_64.egg/astropy/units/format/generic.py", line 401, in parse
result = self._do_parse(s, debug=debug)
File "/Users/deil/Library/Python/3.4/lib/python/site-packages/astropy-1.1.dev12994-py3.4-macosx-10.10-x86_64.egg/astropy/units/format/generic.py", line 423, in _do_parse
"Syntax error parsing unit '{0}'".format(s))
ValueError: Syntax error parsing unit '1/m/s'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/deil/Library/Python/3.4/lib/python/site-packages/astropy-1.1.dev12994-py3.4-macosx-10.10-x86_64.egg/astropy/units/core.py", line 1810, in __call__
raise ValueError(msg)
ValueError: '1/m/s' did not parse as fits unit: Syntax error parsing unit '1/m/s'This is confusing and maybe it would be better to always make this work or always raise an error?
For reference, the FITS unit standard is here:
https://heasarc.gsfc.nasa.gov/docs/heasarc/ofwg/docs/general/ogip_93_001/ogip_93_001.html
EDIT by @mdboom to avoid confusion for others coming across this in the future:
The OGIP unit standard is here: https://heasarc.gsfc.nasa.gov/docs/heasarc/ofwg/docs/general/ogip_93_001/ogip_93_001.html
The FITS unit standard is here:
http://fits.gsfc.nasa.gov/standard30/fits_standard30aa.pdf
Activity
embray commentedon Jul 21, 2015
I think it should always raise an error, if my reading of the FITS units specification is correct (not sure though).
cdeil commentedon Jul 22, 2015
We're adjusting our software to generate these FITS files.
What is the recommended way to encode this unit in FITS?
This parses in Astropy:
But if I'm reading the examples from the standard correctly, this is the recommended encoding, but it fails to parse with Astropy!?
Going through the "recommended way to write compound units examples" in the FITS standard, there's some others that don't parse with Astropy, e.g.
Is there a bug in the parser or does it only cover a subset of FITS units?
embray commentedon Jul 22, 2015
The OGIP unit spec is not the same as the unit spec defined in the FITS standard. It is similar, but subtly different. I'll let @mdboom explain: #1911 (comment)
Anyways:
I wouldn't use the OGIP unit convention unless there is an operational requirement for it.
embray commentedon Jul 22, 2015
FWIW the FITS Standard as defined by the IAU FITS WG is found here: http://fits.gsfc.nasa.gov/fits_standard.html The current version (3.0) defines the current recommendations for formatting units in FITS. The OGIP convention long predates any attempt to systematically describe a unit formatting convention in the FITS Standard itself. But for new FITS files that don't have to conform to the OGIP convention (e.g. for inclusion in an existing archive that uses it consistently), the FITS Standard should be the reference on units in FITS.
Of course, since there is only one version of FITS there should never be any ambiguity about what unit formatting convention a given FITS file is using. [And yes, that should be read dripping with sarcasm :)]
embray commentedon Jul 22, 2015
One thing we might consider doing (I feel like there's been an issue about this before, but I'm not sure), is add a special case when parsing a unit with
format='fits', such that when it fails, fall back on trying to parse it withformat='ogip'instead (and maybe issue a warning?).embray commentedon Jul 22, 2015
(Ah, I wrote something similar here--there I was talking about unit integration directly in
io.fitsitself, though it might be worth having a fallback directly inUnit(...)as well, if it can be done without too much baggage.)mdboom commentedon Jul 22, 2015
@embray: Yes, I recall discussing a fallback somewhat like that before, but I don't think it ever went anywhere. I'm not opposed to something like that.
I recommend always using this form:
MeV^-1 s^-1 sr^-1. It's the most broadly compatible and has no parsing ambiguities -- assuming the units themselves are understood.embray commentedon Jul 22, 2015
@mdboom If you want, I can work on this. I need to dig more into the unit code anyways as I figure out how to best handle units in fitting...
mdboom commentedon Jul 22, 2015
Sure. I guess the design question is whether to have the existing
fitsformat fallback toogipif it fails or add a new format "generic_fits" (or a better name) that tries each. I think there's still a use case for "I want to make sure this unit conforms to the FITS spec". One could also havefitstry both, and rename the existingfitstostrict_fits.embray commentedon Jul 22, 2015
Maybe instead of
strict_fits,iau_fits? Or even justfits_standard?mdboom commentedon Jul 22, 2015
Sure -- either of those names is better.
cdeil commentedon Jul 22, 2015
@embray @mdboom - Thanks for the explanations!
And sorry for the confusion about the OGIP and FITS units standards ... I wasn't aware.
What a mess ... two units standards in FITS files, with no clear way to declare which standard a given file uses?
Under these circumstances, +1 to the suggestion to fall back to
format='ogip'forformat='fits'to make reading work for more FITS files (even if a bit ambiguous).For now it's not clear if we'll use OGIP or FITS, so we'll probably use this for now, which seems to be valid and the same in both standards:
embray commentedon Jul 22, 2015
No need to apologize, any reasonable person would be confused about this :)
embray commentedon Jul 22, 2015
I'm still under the impression that the fact that
1 /is being accepted by the'fits'format is incorrect.mdboom commentedon Jul 22, 2015
Yes, the fact that
1 /works in the FITS format now is a bug -- to be fixed in astropy 1.1: #3993.embray commentedon Jul 22, 2015
@mdboom Thanks, I didn't realize that was fixed by #3993 too.
cdeil commentedon Jul 22, 2015
What about the fallback to OGIP?
It sounded like you were both in favour or at least OK with doing this for
format='fits'?Does that need discussion on astropy-dev or something, or would you feel comfortable just making this change for
format='fits'?embray commentedon Jul 22, 2015
I'm going to add a PR for that in a bit. I think we can just change
format='fits'to try standard FITS first then fall back to OGIP if that works.mhvk commentedon Aug 11, 2016
Looking over old units issues, I came across this one. My own feeling is that the behaviour in
unitsitself is fine: if you ask for something to be interpreted asfits, it should follow that convention. However, in opening and interpreting fits headers one might well want to fall back onogip. So, adding anio.fitslabel...bsipocz commentedon Mar 18, 2019
Unit('1/m', format='fits')raises a sensible exception for a while now (ValueError: '1/m' did not parse as fits unit: Numeric factor not supported by FITS) and based on the comments above it is the right behaviour so I'm closing this issue.Please reopen if you disagree and think there is something to be changed in astropy.