FITS unit parsing with "1 / ..." sometimes fails

[cdeil]

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

cc @embray @mdboom @mapazarr

[embray]

I think it should always raise an error, if my reading of the FITS units specification is correct (not sure though).

[cdeil]

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:

>>> Unit('MeV^-1 s^-1 sr^-1', format='fits')
Unit("1 / (MeV s sr)")
>>> Unit('1/(MeV s sr)', format='fits')
Unit("1 / (MeV s sr)")
>>> Unit('count/(MeV s sr)', format='fits')
Unit("ct / (MeV s sr)")

But if I'm reading the examples from the standard correctly, this is the recommended encoding, but it fails to parse with Astropy!?

>>> Unit('/MeV /s /sr', format='fits')
ValueError: '/MeV /s /sr' did not parse as fits unit: Syntax error parsing unit '/MeV /s /sr'

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.

>>> Unit('/pixel /s', format='fits')
ValueError: '/pixel /s' did not parse as fits unit: Syntax error parsing unit '/pixel /s'

Is there a bug in the parser or does it only cover a subset of FITS units?

[embray]

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:

In [2]: Unit('/MeV /s /sr', format='ogip')
Out[2]: Unit("1 / (MeV s sr)")

In [3]: Unit('/pixel /s', format='ogip')
Out[3]: Unit("1 / (pix s)")

I wouldn't use the OGIP unit convention unless there is an operational requirement for it.

[embray]

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]

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 with format='ogip' instead (and maybe issue a warning?).

[embray]

(Ah, I wrote something similar here--there I was talking about unit integration directly in io.fits itself, though it might be worth having a fallback directly in Unit(...) as well, if it can be done without too much baggage.)

[mdboom]

@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]

@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]

Sure. I guess the design question is whether to have the existing fits format fallback to ogip if 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 have fits try both, and rename the existing fits to strict_fits.

[embray]

Maybe instead of strict_fits, iau_fits? Or even just fits_standard?

[mdboom]

Sure -- either of those names is better.

[cdeil]

@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' for format='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:

In [5]: Unit('1 / (MeV s sr)', format='ogip')
Out[5]: Unit("1 / (MeV s sr)")

In [6]: Unit('1 / (MeV s sr)', format='fits')
Out[6]: Unit("1 / (MeV s sr)")

[embray]

And sorry for the confusion about the OGIP and FITS units standards ... I wasn't aware.

No need to apologize, any reasonable person would be confused about this :)

[embray]

I'm still under the impression that the fact that 1 / is being accepted by the 'fits' format is incorrect.

[mdboom]

Yes, the fact that 1 / works in the FITS format now is a bug -- to be fixed in astropy 1.1: #3993.

[embray]

@mdboom Thanks, I didn't realize that was fixed by #3993 too.

[cdeil]

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]

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]

Looking over old units issues, I came across this one. My own feeling is that the behaviour in units itself is fine: if you ask for something to be interpreted as fits, it should follow that convention. However, in opening and interpreting fits headers one might well want to fall back on ogip. So, adding an io.fits label...

[bsipocz]

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.