Enable parsing of mag, dB, dex

[mhvk]

This is follow-up on #1894, enabling the parsing of logarithmic units for the generic format (it builds on that PR, so just look at the last commit). It allows:

In [2]: u.Unit('dB(mW)')
Out[2]: DecibelUnit('mW')

In [3]: u.Decibel(10., 'dB(mW)')
Out[3]: <Decibel 10.0 dB(mW)>

@mdboom - could you check I do the parsing correctly? I also continue to wonder whether somehow the function units shouldn't be a Unit subclass after all (or of a more abstract unit base class).

Another thing I considered is whether it would make sense to make the existing u.mag, u.dB, and u.dex special units that have a __call__ method, so that u.dB(u.mW) would give a u.DecibelUnit.

[pllim]

Personally, I would find __call__ confusing in this context. I rather use a more descriptive method.

[mhvk]

@mdboom - well, travis shows that I did not do the parsing correctly: now, dex, dB, and mag are no longer interpreted as units if there is anything more than just their name in the string (i.e., u.Unit("mag") works, but not u.Unit("mag ")).

[mhvk]

@pllim - the explicit method does already exist (u.DecibelUnit('mW')). I just seemed nice to be able to short-circuit this with u.dB(u.mW) -- but this is not a big deal.

[mhvk]

@mdboom - I think I fixed the regex such that a function name will be interpreted as a unit if not followed by a parenthesis. Hence,

In [2]: u.Unit('dB / m')
Out[2]: Unit("dB / m")

In [3]: u.Unit('dB(mW)')
Out[3]: DecibelUnit('mW')

In [4]: u.Unit('dB (mW)')
Out[4]: DecibelUnit('mW')

[mdboom]

The regex fix looks fine. Can you add a test with dB (mW) in it?

[mdboom]

This line needs to stay (even though PLY doesn't generate it) so that the tokens are unicode.

[mhvk]

@mdboom - on both this comment and that on the appearance of the full path below: is there a way to automate this? I'll make the changes by hand here, but it seems error prone (and it'll have to be done again for unicode, etc.).

[mdboom]

Personally, I like the suggestion of making dB(mW) work. @pllim: Can you explain your confusion? Seems non-ambiguous to me.

[mdboom]

This seems like a much slower test than before. Why is it needed?

[mhvk]

See https://github.com/astropy/astropy/pull/1894/files#r8301896 -- the present PR is a follow-up on that, just adding the parsing. It also relates to item 3 in the list of outstanding questions for the function units (https://github.com/astropy/astropy/pull/1894/files#r8301896).

[pllim]

@mhvk , @mdboom , if it's clear to everyone else, then you can ignore my comment.

[mhvk]

Hmm, adding tests as always is useful: it seems as written, any function can only take a single unit or a product of units in parentheses. This can be seen in master using sqrt: u.Unit('sqrt(m s)') does not work, 'sqrt((m s))' does work, 'sqrt((m/s))' does not work. @mdboom - would you have any suggestions?

[mhvk]

On the callable magnitudes, etc., this is quite trivial to implement -- see proof of concept in #3618. But also brought to the fore some problems -- see main PR #1894.

[mhvk]

Mostly for my own sake: to do here is to either disallow mag, dB',dex` in FITS, VOTable, OGIP, or make them work to the extend the standard allows (thus solving #3822).

[mhvk]

@mdboom - I now rebased the parser and it seems to work fine for Generic (I added more tests as well).

For FITS and VOUnit, I've been blunt for now, and simply ensured that any function unit raises ValueError. As I'll need to read up on exactly how FITS and VOUnit support log, which will need a bit more time, I'd rather postpone that to #3822, so that we can be sure that the generic parsing is in for 1.1.

[mhvk]

p.s. I quickly looked at the documentation, to see whether I should make changes, and I think with just this PR it is probably best to leave it as is. But if one also includes the callable units (#3618), the use of normal or function units becomes quite automatic and it would be logical to start omitting the whole DexUnit stuff in the documentation. instead, function units should just represent themselves as Unit("dex(cm / s2)"), and be created with that call or with u.dex(u.cm/u.s**2). The added advantage would be that we can change the API more easily...

[mdboom]

I concur about putting off #3822. It's not a huge priority, as I don't think many other tools use functional units in those formats either.

I also think removing the DexUnit stuff from the documentation in favor of the more natural callable functions is a good idea.

[mhvk]

Am closing this as it is part of #3618 (which also introduces the callable u.mag, etc.) and they would seem easiest reviewed as a package.