Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pitfalls of "unknown units' support in VOUnits #8978

Open
dhomeier opened this issue Jul 10, 2019 · 1 comment
Open

Pitfalls of "unknown units' support in VOUnits #8978

dhomeier opened this issue Jul 10, 2019 · 1 comment

Comments

@dhomeier
Copy link
Contributor

The full support of the VOUnit standard with #2901 has led to some confusion about its acceptance of unrecognised or unknown units (§ 2.11 of the standard) as in #6302 and #8959.

Essentially u.Unit("name", format='vounit') is just following quite literally the 1.0 standard by allowing, even implicit, construction of arbitrary 'unknown units'. These are not instances of outr units's UnrecognizedUnit class, since per the standard they still accept SI prefixes. Handling is further complicated by the requirement that any valid SI prefix shall always be recognised in a plain string, while unit names in literal single quotes must not be further parsed. As per the example in the standard

  • "furlong" is parsed as "femto-urlong", with .bases[0]._represents Unit("1e-15 urlong")
  • "'furlong'" is parsed as "'furlong'", being an IrreducibleUnit
  • "f'furlong'" would be parsed as "femto-'furlong'".

Again, this is rather confusing to most users not intimately familiar with the VO standards, and since the quoted custom units are parsed without any notification, others with only a one-time warning, prone to coding errors. In Astropy itself there is some ambiguity at least in CHANGES.rst, which only mentions the inline definition of units in quotes, and in the test of u.Unit('ANGSTROM', format='vounit') which seems to suggest that it should raise a ValueError, but actually passes without an exception.

My recommendation would be to raise all warnings by one level, i.e. issue a warning on uses of the quoted syntax like "m'ANGSTROM'", "'kibibytes'" as well, and allow other custom units only per explicit definition. The most straightforward way would be to accept names like "furlong/week" only with parse_strict='warn' or 'silent' set as for the other parsers, but since these cases are handled by generic.py, that would break the current compliance with the prefix treatment (the units would become UnrecognizedUnit instances).
One solution might be adding a fourth parse_strict setting to return a regular unit instead, but maybe a better way would be adding an additional syntax in votable.py to recognise double-quoted strings, to make this work:

myunit = u.Unit("'furlong'", format='vounit')
WARNING: UnitsWarning: Unit 'furlong' is not recognized in the VOUnit standard.  [astropy.units.format.vounit]
>>> myunit.names
['furlong', "'furlong'"]
>>> isinstance(myunit, u.IrreducibleUnit)
True
myunit =  = u.Unit('"furlong"', format='vounit')
WARNING: UnitsWarning: Unit "furlong" is not recognized in the VOUnit standard.  [astropy.units.format.vounit]
>>> myunit.names
['furlong']
>>> isinstance(myunit, u.IrreducibleUnit)
False
>>> myunit.bases[0]._represents
Unit("1e-15 urlong")

Actually takes just a 2-character change to _custom_unit_regex. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants