VOUnit to issue VOWarning rather than generic UnitsWarning

[bsipocz]

astropy.io.votable.exceptions.W50 is defined to be the warning for units that do not adhere to the VO standards: https://www.ivoa.net/documents/VOUnits

yet, astropy.units.format.vounit.VOUnit issues a generic UnitsWarning for the exact same reason.

This is somewhat unexpected in practice, and while in the example below we could ignore both, I don't think that's the right approach:

astropy/astroquery#2352

[pllim]

What would be a desired behavior here?

[bsipocz]

To use astropy.io.votable.exceptions.W50 instead of UnitsWarning in e.g. astropy/units/format/vounit.py (and to double-check the rest of file for warnings and errors and use the existing VO specific ones if possible):

            if cls._custom_unit_regex.match(t.value):
                warnings.warn(
                    "Unit {!r} not supported by the VOUnit "
                    "standard. {}".format(
                        t.value, utils.did_you_mean_units(
                            t.value, cls._units, cls._deprecated_units,
                            cls._to_decomposed_alternative)),
                    core.UnitsWarning)

[pllim]

I am not sure if it will be a drop-in replacement in terms of the warning behavior. As you can see here:

astropy/astropy/io/votable/exceptions.py

Lines 1040 to 1052 in 6c15aa6

	class W50(VOTableSpecWarning):
	"""
	Invalid unit string as defined in the `Units in the VO, Version 1.0
	<https://www.ivoa.net/documents/VOUnits>`_ (VOTable version >= 1.4)
	or `Standards for Astronomical Catalogues, Version 2.0
	<http://cdsarc.u-strasbg.fr/doc/catstd-3.2.htx>`_ (version < 1.4).
	Consider passing an explicit ``unit_format`` parameter if the units
	in this file conform to another specification.
	"""
	message_template = "Invalid unit string '{}'"
	default_args = ('x',)

It is really customized towards displaying a particular XML format that goes beyond VO unit specification. If you pass it in as warning class, you will see some things that only makes sense if you also pass in things very specific to VO table parsing, like its configuration and position that the unit appears in the table. Is that what you really want?

>>> import warnings
>>> from astropy.io.votable.exceptions import W50
>>> warnings.warn('foo', W50)
WARNING: W50: ?:?:?: W50: Invalid unit string 'foo' [warnings]

When you use W50 in the actual XML validation, it displays something like this:

WARNING: W50: vo.xml:154:0: W50: Invalid unit string 'pixel'

[bsipocz]

Then what about making this a new UnitsWarning, that is a subclass of W50? That would make the suppressing happy and can come with whatever custom text that makes sense for the units module.

[pllim]

Looking at the inheritance tree of these two warnings, UnitsWarning is higher up in the chain than W50. I am not sure if having yet another warning class is a good idea. Let's loop in the units maintainers (@nstarman , @mhvk , @adrn ) and see what they think.

(Warning -> AstropyWarning -> VOWarning) + SyntaxWarning -> VOTableSpecWarning -> W50

Warning -> AstropyWarning -> UnitsWarning

[nstarman]

I think both concerns — that incorrect VOUnits should warn with W50 and that we have a lot of warning classes — are reasonable. In this case units is working for compatibility with the VO standards, so my inclination would be to use the W50 + UnitsWarning subclass.

Instead of subclassing W50, would it be possible to make W50 itself a subclass of UnitsWarning and control the contents of the message depending on where the warning was raised?
ie warnings.warn('foo', W50) should not show configuration and position information, which should be shown when parsing a table. If this is possible, having W50 be a UnitsWarning subclass would be my preference.

[bsipocz]

I don't see any issues with having multiple inheritances for W50, but @tomdonaldson needs to comment on that from the VO side.

[pllim]

I hope we are not over-engineering this...

make W50 itself a subclass of UnitsWarning

Theoretically possible but it is starting to hurt my head since it already mixing in two different classes to begin with.

control the contents of the message depending on where the warning was raised?

I cannot think of a simple way to do this. I think at the very least you will have to rewrite its constructor to handle this logic that is currently defined up in VOWarning:

astropy/astropy/io/votable/exceptions.py

Line 217 in 6c15aa6

self.formatted_message = _format_message(

[eerovaher]

If we look at the code that parses the VOTable then we see that it should already properly replace any UnitsWarning instances with W50:

astropy/astropy/io/votable/tree.py

Lines 1390 to 1404 in acdefa2

	# First, parse the unit in the default way, so that we can
	# still emit a warning if the unit is not to spec.
	default_format = _get_default_unit_format(self._config)
	unit_obj = u.Unit(
	unit, format=default_format, parse_strict='silent')
	if isinstance(unit_obj, u.UnrecognizedUnit):
	warn_or_raise(W50, W50, (unit,),
	self._config, self._pos)
	format = _get_unit_format(self._config)
	if format != default_format:
	unit_obj = u.Unit(
	unit, format=format, parse_strict='silent')
	self._unit = unit_obj

The fact that we still get an UnitsWarning despite parse_strict='silent' means that there is a bug that would not be fixed by changing the warnings hierarchy, so that does not sound like a solution we should pursue. Indeed, the underlying bug can be easily confirmed if we try out different unit formats. With e.g. format='cds' everything works as expected:

>>> from astropy import units as u
>>> u.Unit('e-/s', format='cds', parse_strict='raise')
...
ValueError: 'e-/s' did not parse as cds unit: Syntax error If this is meant to be a custom unit...
>>> u.Unit('e-/s', format='cds', parse_strict='warn')
WARNING: UnitsWarning: 'e-/s' did not parse as cds unit: Syntax error If this is meant to be a custom unit...
UnrecognizedUnit(e-/s)
>>> u.Unit('e-/s', format='cds', parse_strict='silent')
UnrecognizedUnit(e-/s)

But now with format='vounit':

>>> u.Unit('e-/s', format='vounit', parse_strict='raise')
WARNING: UnitsWarning: Unit 'e' not supported by the VOUnit standard.  [astropy.units.format.vounit]
...
ValueError: 'e-/s' did not parse as vounit unit: Invalid character at col 1 If this is meant to be a custom unit...
>>> u.Unit('e-/s', format='vounit', parse_strict='warn')
WARNING: UnitsWarning: Unit 'e' not supported by the VOUnit standard.  [astropy.units.format.vounit]
WARNING: UnitsWarning: 'e-/s' did not parse as vounit unit: Invalid character at col 1 If this is meant to be a custom unit...
UnrecognizedUnit(e-/s)
>>> u.Unit('e-/s', format='vounit', parse_strict='silent')
WARNING: UnitsWarning: Unit 'e' not supported by the VOUnit standard.  [astropy.units.format.vounit]
UnrecognizedUnit(e-/s)

We can see that using format='vounit' always results in an additional warning, which is the real bug that needs to be fixed.

[nstarman]

Thanks for the sleuthing @eerovaher!

[pllim]

@eerovaher , thanks for illustrating the bug nicely. Is anyone pursuing a fix? Not sure when I can get to this but I also don't want to duplicate work if you already know how to fix it. 😉

[orionlee]

I accidentally came across a small test case that might narrow down the source of the warning.

The UnitsWarning has been emitted before parse_strict argument is processed. See the following example when I supply an incorrect parse_strict value, the UnitsWarning is there.

>>> from astropy import units as u
>>> u.Unit('e-/s', format='vounit', parse_strict='foobar')
WARNING: UnitsWarning: Unit 'e' not supported by the VOUnit standard.  [astropy.units.format.vounit]

Traceback (most recent call last):
  File "C:\dev\astropy\astropy\units\format\generic.py", line 612, in _do_parse
    return cls._parse_unit(s, detailed_exception=False)
  File "C:\dev\astropy\astropy\units\format\vounit.py", line 124, in _parse_unit
    raise ValueError()
ValueError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "C:\dev\astropy\astropy\units\core.py", line 2019, in __call__
    return f.parse(s)
  File "C:\dev\astropy\astropy\units\format\vounit.py", line 94, in parse
    result = cls._do_parse(s, debug=debug)
  File "C:\dev\astropy\astropy\units\format\generic.py", line 615, in _do_parse
    return cls._parser.parse(s, lexer=cls._lexer, debug=debug)
  File "C:\dev\astropy\astropy\utils\parsing.py", line 115, in parse
    return self.parser.parse(*args, **kwargs)
  File "C:\dev\astropy\astropy\extern\ply\yacc.py", line 333, in parse
    return self.parseopt_notrack(input, lexer, debug, tracking, tokenfunc)
  File "C:\dev\astropy\astropy\extern\ply\yacc.py", line 1063, in parseopt_notrack
    lookahead = get_token()     # Get the next token
  File "C:\dev\astropy\astropy\extern\ply\lex.py", line 386, in token
    newtok = self.lexerrorf(tok)
  File "C:\dev\astropy\astropy\units\format\generic.py", line 159, in t_error
    raise ValueError(
ValueError: Invalid character at col 1

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\dev\astropy\astropy\units\core.py", line 2045, in __call__
    raise ValueError("'parse_strict' must be 'warn', "
ValueError: 'parse_strict' must be 'warn', 'raise' or 'silent'

[eerovaher]

Is anyone pursuing a fix? Not sure when I can get to this but I also don't want to duplicate work if you already know how to fix it.

The fix does not seem to be trivial, I don't have a working solution.

[pllim]

@orionlee 's clue is very valuable. I might have a fix and will submit a PR shortly submitted #13042 . Just trying to see if there is a sane way to test it. Thanks!

[pllim]

I have proposed a fix at #13042 but it comes with some behavior change. I dunno if it is good or bad. Please have a look and discuss. Thanks!

[eerovaher]

Turns out the bug we uncovered here was reported already years ago: #6302