FITS unit format fixes by mdboom · Pull Request #3993 · astropy/astropy

mdboom · 2015-07-22T15:53:48Z

- Accept power-of-10 scales in FITS units (and only power-of-10 scales) - Accept * as a separator between the scale factor and the units

embray · 2015-07-22T17:59:17Z

I feel like this should be a 1.1 fix, since it could cause existing code to break, maybe? What do you think?

embray · 2015-07-22T18:01:16Z

Actually, looking back at the original bug report (I didn't see that before) it's a little more clear to me now what's going on here--that units that should have been accepted aren't.

embray · 2015-07-22T18:03:33Z

This not not conformant with the standard though. The intent is unambiguous so I don't mind accepting it but this could be pointing out (the standard here is a little ambiguous too, but if you dig in it seems to imply that this is not allowed).

Yeah -- I agree. I don't think this is explicitly in the standard, though if you read only the operator table, you might think that it was. As it's unambiguous, I'm applying the "be liberal in what you accept" rule. But I'll add a comment to this effect.

mdboom · 2015-07-22T18:25:54Z

Yeah -- I'm waffling about whether this is 1.1 or 1.0.4. It does actually break units that used to be accepted, i.e.:

master:

>>> u.Unit('1.2 m', format='fits')
1.2 m

This PR:

>>> u.Unit('1.2 m', format='fits')
ValueError: '1.2 m' did not parse as fits unit: Numeric factor not supported by FITS

So maybe this should be 1.1 after all?

embray · 2015-07-22T18:52:31Z

Okay, let's do 1.1 then.

embray · 2015-07-22T18:52:55Z

Or could still accept the arbitrary scale with a deprecation warning?

mdboom · 2015-07-22T18:57:39Z

As this is all so closely packed, I think it's easiest to just make this 1.1.

embray · 2015-07-22T19:05:04Z

Okay, fine by me.

mdboom · 2015-07-23T12:49:09Z

@saimn: Can you confirm this works for you? Once that's done, I think this is good to go.

mhvk · 2015-07-23T22:28:43Z

@mdboom - looks good to me! This means percent now can be written to fits, correct?

mdboom · 2015-07-23T22:36:11Z

Yes, percent can now be written:

In [2]: u.percent.to_string(format="fits")
Out[2]: u'10**-2'

saimn · 2015-07-24T11:08:24Z

@mdboom I have tested several formats, '10**(-20)*erg/s/cm**2/Angstrom' and '10**(-20) erg/s/cm**2/Angstrom' are correctly parsed, though it doesn't work without the parenthesis around the exponent. It is not an issue for my case, and the description in the standard is not clear about this but it seems it should also be supported ?

saimn · 2015-07-24T11:15:12Z

And as to_string gives a string without the parenthesis this should also be supported, otherwise it is not possible to parse the unit written with astropy:

In [1]: from astropy import units as u

In [2]: unit = u.Unit('10**(-20)*erg/s/cm**2/Angstrom', format='fits')
WARNING: UnitsWarning: The unit 'erg' has been deprecated in the FITS standard. Suggested: cm2 g s-2. [astropy.units.format.utils]
WARNING: UnitsWarning: The unit 'Angstrom' has been deprecated in the FITS standard. Suggested: 10**-1 nm. [astropy.units.format.utils]
WARNING: UnitsWarning: '10**(-20)*erg/s/cm**2/Angstrom' contains multiple slashes, which is discouraged by the FITS standard [astropy.units.format.generic]

In [3]: u.Unit(unit.to_string('fits'), format='fits')
WARNING: UnitsWarning: The unit 'erg' has been deprecated in the FITS standard. Suggested: cm2 g s-2. [astropy.units.format.utils]
WARNING: UnitsWarning: The unit 'Angstrom' has been deprecated in the FITS standard. Suggested: 10**-1 nm. [astropy.units.format.utils]
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
/home/simon/lib/astropy/astropy/units/format/generic.py in _do_parse(self, s, debug)
    445             # is just a single unit name
--> 446             return self._parse_unit(s, detailed_exception=False)
    447         except ValueError as e:

/home/simon/lib/astropy/astropy/units/format/fits.py in _parse_unit(cls, unit, detailed_exception)
    102     def _parse_unit(cls, unit, detailed_exception=True):
--> 103         cls._validate_unit(unit)
    104         return cls._units[unit]

/home/simon/lib/astropy/astropy/units/format/fits.py in _validate_unit(cls, unit, detailed_exception)
     91                             unit, cls._units, cls._deprecated_units,
---> 92                             cls._to_decomposed_alternative)))
     93             else:

ValueError: Unit '10**-20 Angstrom-1 cm-2 erg s-1' not supported by the FITS standard. 

During handling of the above exception, another exception occurred:

TypeError                                 Traceback (most recent call last)
/home/simon/lib/astropy/astropy/units/core.py in __call__(self, s, represents, format, namespace, doc, parse_strict)
   1795             try:
-> 1796                 return f.parse(s)
   1797             except Exception as e:

/home/simon/lib/astropy/astropy/units/format/fits.py in parse(self, s, debug)
    157     def parse(self, s, debug=False):
--> 158         result = super(Fits, self).parse(s, debug)
    159         if hasattr(result, 'function_unit'):

/home/simon/lib/astropy/astropy/units/format/generic.py in parse(self, s, debug)
    432 
--> 433         result = self._do_parse(s, debug=debug)
    434         if s.count('/') > 1:

/home/simon/lib/astropy/astropy/units/format/generic.py in _do_parse(self, s, debug)
    448             try:
--> 449                 return self._parser.parse(s, lexer=self._lexer, debug=debug)
    450             except ValueError as e:

/home/simon/lib/astropy/astropy/extern/ply/yacc.py in parse(self, input, lexer, debug, tracking, tokenfunc)
    264         else:
--> 265             return self.parseopt_notrack(input,lexer,debug,tracking,tokenfunc)
    266 

/home/simon/lib/astropy/astropy/extern/ply/yacc.py in parseopt_notrack(self, input, lexer, debug, tracking, tokenfunc)
    970                             del statestack[-plen:]
--> 971                             p.callable(pslice)
    972                             symstack.append(sym)

/home/simon/lib/astropy/astropy/units/format/generic.py in p_factor_fits(p)
    254                 else:
--> 255                     p[0] = 10 ** (p[2] * p[3])
    256             elif len(p) == 5:

TypeError: unsupported operand type(s) for *: 'NoneType' and 'float'

During handling of the above exception, another exception occurred:

ValueError                                Traceback (most recent call last)
<ipython-input-3-9df61ff3f84f> in <module>()
----> 1 u.Unit(unit.to_string('fits'), format='fits')

/home/simon/lib/astropy/astropy/units/core.py in __call__(self, s, represents, format, namespace, doc, parse_strict)
   1808                            .format(s, format_clause, six.text_type(e)))
   1809                     if parse_strict == 'raise':
-> 1810                         raise ValueError(msg)
   1811                     elif parse_strict == 'warn':
   1812                         warnings.warn(msg, UnitsWarning)

ValueError: '10**-20 Angstrom-1 cm-2 erg s-1' did not parse as fits unit: unsupported operand type(s) for *: 'NoneType' and 'float'

mdboom · 2015-07-24T12:47:13Z

Yeah -- the exponent without the parentheses was just broken by accident. A fix and additional tests have been added here.

embray · 2015-07-24T18:00:59Z

Looks like this is good to go. @saimn , do want to do any more testing or are you satisfied by the changes?

saimn · 2015-07-24T22:05:30Z

Looks good to me, thanks for the quick iterations on this Michael.

FITS unit format fixes

mdboom added 3 commits July 22, 2015 11:38

Fix astropy#3991: FITS unit format fixes

b41ec16

- Accept power-of-10 scales in FITS units (and only power-of-10 scales) - Accept * as a separator between the scale factor and the units

Fix tests

b4a924a

Add CHANGES entry

c283807

mdboom mentioned this pull request Jul 22, 2015

FITS, Units and numeric multiplier #3991

Closed

embray added units Bug Affects-release labels Jul 22, 2015

embray reviewed Jul 22, 2015
View reviewed changes

Remove local path from comment

988fa5f

mdboom added this to the v1.1.0 milestone Jul 22, 2015

Move to API Changes for 1.1

2678210

mdboom mentioned this pull request Jul 22, 2015

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

Closed

Fix error message

15f2143

Fix FITS scale exponent without parentheses

2150c31

mdboom added a commit that referenced this pull request Jul 25, 2015

Merge pull request #3993 from mdboom/unit/fits-unit-format-fixes

47f6eba

FITS unit format fixes

mdboom merged commit 47f6eba into astropy:master Jul 25, 2015

Conversation

mdboom commented Jul 22, 2015

Uh oh!

embray commented Jul 22, 2015

Uh oh!

embray commented Jul 22, 2015

Uh oh!

embray Jul 22, 2015

Choose a reason for hiding this comment

Uh oh!

mdboom Jul 22, 2015

Choose a reason for hiding this comment

Uh oh!

mdboom commented Jul 22, 2015

Uh oh!

embray commented Jul 22, 2015

Uh oh!

embray commented Jul 22, 2015

Uh oh!

mdboom commented Jul 22, 2015

Uh oh!

embray commented Jul 22, 2015

Uh oh!

mdboom commented Jul 23, 2015

Uh oh!

mhvk commented Jul 23, 2015

Uh oh!

mdboom commented Jul 23, 2015

Uh oh!

saimn commented Jul 24, 2015

Uh oh!

saimn commented Jul 24, 2015

Uh oh!

mdboom commented Jul 24, 2015

Uh oh!

embray commented Jul 24, 2015

Uh oh!

saimn commented Jul 24, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants