Skip to content

Improve whitespace handling in astropy.unit.Unit FITS format string parsing? - #602

Merged
mdboom merged 1 commit into
astropy:masterfrom
mdboom:unit/fits-parser-whitespace-handling
Jan 4, 2013
Merged

mdboom merged 1 commit into
astropy:masterfrom
mdboom:unit/fits-parser-whitespace-handling

Conversation

@mdboom

@mdboom mdboom commented Jan 3, 2013

Copy link
Copy Markdown
Contributor

I ran into this error:

In [81]: u.Unit('m / s')
ERROR: ValueError: Expected integer (at char 5) in 'm / s' [astropy.units.format.generic]
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-81-ee9c94fa4475> in <module>()
----> 1 u.Unit('m / s')

/Users/deil/Library/Python/2.7/lib/python/site-packages/astropy/units/core.pyc in __call__(self, s, represents, format, register, doc, parse_strict)
    735             f = unit_format.get_format(format)
    736             try:
--> 737                 return f.parse(s)
    738             except ValueError as e:
    739                 if parse_strict == 'raise':

/Users/deil/Library/Python/2.7/lib/python/site-packages/astropy/units/format/generic.pyc in parse(self, s)
    247             except p.ParseException as e:
    248                 raise ValueError("{0} in {1!r}".format(
--> 249                     utils.cleanup_pyparsing_error(e), s))
    250 
    251     def _get_unit_name(self, unit):

ValueError: Expected integer (at char 5) in 'm / s'

It took me a minute to figure out that the spaces are the problem, this works:

In [82]: u.Unit('m/s')
Out[82]: Unit("m / (s)")

After reading Section 4.3.1 on "Construction of units strings" of the FITS Standard I realize that the spaces in the 'm / s' unit string is discouraged and thus the behaviour of the astropy parser is not a bug:

Parentheses are used for symbol grouping and are strongly recommended whenever the order of operations might be subject to misinterpretation. A space character implies multiplication which can also be conveyed explicitly with an asterisk or a period. Therefore, although spaces are allowed as symbol separators, their use is discouraged.

However, @mdboom, I was wondering if the parsing error or maybe even the parsing behaviour could be improved here. It is a bit confusing that spaces in the unit string are OK in some cases:

u.Unit('m /s') # space before is OK
u.Unit('m / s1') # space before and after are OK if second operator has an exponent
u.Unit('m / (s)') # spaces before and after are OK if second operator is in parentheses
u.Unit('m    /  s1') # multiple spaces are OK where one space is OK
u.Unit('m s') # space only means multiply, i.e. u.Unit('m*s')

But spaces are not OK in these cases

u.Unit('m *s') # space before * not OK, was OK for /
u.Unit('m/ s') # space after / is not OK

Is it possible (with reasonable effort) to make u.Unit('m / s') work or give a more explicit error message that whitespace is the problem? If not, obviously feel free to just close this issue since this is not a bug.

Note that the CDS parser has it easy and gives

In [113]: u.Unit('m / s', format='cds')
ERROR: ValueError: CDS unit must not contain whitespace [astropy.units.format.cds]

@mdboom

mdboom commented Jan 3, 2013

Copy link
Copy Markdown
Contributor

Yeah -- that "whitespace as an operator" trick creates a lot of surprising opportunities for ambiguity in the parser, but I think this case is not one of them, so I've just gone ahead and updated it so accept whitespace after the solidus.

mdboom added a commit that referenced this pull request Jan 4, 2013
Improve whitespace handling in astropy.unit.Unit FITS format string parsing?
@mdboom
mdboom merged commit 95ffef3 into astropy:master Jan 4, 2013
mdboom added a commit that referenced this pull request Jan 4, 2013
Improve whitespace handling in astropy.unit.Unit FITS format string parsing?
keflavich pushed a commit to keflavich/astropy that referenced this pull request Oct 9, 2013
…e-handling

Improve whitespace handling in astropy.unit.Unit FITS format string parsing?
@mdboom
mdboom deleted the unit/fits-parser-whitespace-handling branch May 21, 2014 23:57
@cdeil

cdeil commented Jul 21, 2015

Copy link
Copy Markdown
Member Author

@mdboom – There's another FITS unit parsing error that @mapazarr has run into at gammapy/gammapy#299 (comment)

I think '1/s/MeV/sr' should parse as a FITS unit (even if discouraged according to the standard)?
It currently gives this error: https://gist.github.com/cdeil/64707c79858d436e1481

@embray

embray commented Jul 21, 2015

Copy link
Copy Markdown
Member

@cdeil I think the problem here is the 1/ part, which if my reading is correct (it may not be) is not actually allowed in the FITS units format. However, you've also exposed some cases where that's being allowed to slip through, probably unintentionally. You should open a separate issue for this.

@cdeil

cdeil commented Jul 21, 2015

Copy link
Copy Markdown
Member Author

@embray Thanks for the quick comment. Let's continue the discussion here: #3986

jeffjennings pushed a commit to jeffjennings/astropy that referenced this pull request Jul 2, 2025
…first_issue_label

DOC: advertise 'good-first-issue' label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants