Conversation
|
Personally, I would find |
|
@mdboom - well, travis shows that I did not do the parsing correctly: now, |
|
@pllim - the explicit method does already exist ( |
|
@mdboom - I think I fixed the |
|
The regex fix looks fine. Can you add a test with |
There was a problem hiding this comment.
This line needs to stay (even though PLY doesn't generate it) so that the tokens are unicode.
There was a problem hiding this comment.
@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.).
|
Personally, I like the suggestion of making |
astropy/units/core.py
Outdated
There was a problem hiding this comment.
This seems like a much slower test than before. Why is it needed?
There was a problem hiding this comment.
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).
|
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: |
e3a8e11 to
02f8e9b
Compare
|
Mostly for my own sake: to do here is to either disallow |
Lets functions be interpreted as units if not followed by a parenthesis.
02f8e9b to
8086263
Compare
|
@mdboom - I now rebased the parser and it seems to work fine for For |
|
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 |
|
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 |
|
Am closing this as it is part of #3618 (which also introduces the callable |
This is follow-up on #1894, enabling the parsing of logarithmic units for the
genericformat (it builds on that PR, so just look at the last commit). It allows:@mdboom - could you check I do the parsing correctly? I also continue to wonder whether somehow the function units shouldn't be a
Unitsubclass 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, andu.dexspecial units that have a__call__method, so thatu.dB(u.mW)would give au.DecibelUnit.