Repository navigation
FITS unit parsing with "1 / ..." sometimes fails #3986
Description
Activity
I think it should always raise an error, if my reading of the FITS units specification is correct (not sure though).
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?
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.
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 :)]
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: 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.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.Maybe instead of
strict_fits,iau_fits? Or even justfits_standard?Sure -- either of those names is better.
@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 toformat='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:
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)")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 :)
I'm still under the impression that the fact that
1 /is being accepted by the'fits'format is incorrect.What about the fallback to OGIP?
It sounded like you were both in favour or at least OK with doing this forformat='fits'?
Does that need discussion on astropy-dev or something, or would you feel comfortable just making this change forformat='fits'?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.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...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.
As mentioned in #602 (comment), the FITS unit parsing with "1 / .." sometimes works and sometimes fails:
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