Skip to content

Commit 8b124d3

Browse files
eerovahereerovaher
committed
Implement mechanism for more flexible unit parsing
`Unit()` accepts the `parse_strict` argument, which is supposed to let users choose if unit parsing problems cause errors, warnings or are suppressed silently. This has so far been done by letting unit parsers raise errors, which are handled based on the `parse_strict` value. However, raising an error forces parsers to stop early, meaning that every problem becomes a critical error and `Unit` has no choice but to return an `UnrecognizedUnit`. Therefore `parse_strict="raise"` cannot be very strict because otherwise `parse_strict="warn"` and `"silent"` can cause problems for users who might be e.g. reading in files they have little control over. An improvement is to let the unit parsers emit warnings for non-critical problems and to let the machinery in `Unit` handle those warnings based on the `parse_strict` value. Warnings do not force parsers to stop early, so it will be possible to make the `parse_strict="raise"` option stricter without forcing `UnrecognizedUnit` instances with `"warn"` or `"silent"`. Unit parsers still have the option of immediately raising an error in case of serious problems. The new implementation places the warning handling in `_UnitMetaClass` so it is inherited by `Unit`. The other possible implementation without code duplication would be to place it in `Generic._do_parse()`. However, that would require passing the `parse_strict` value through the unit formatter `parse()` methods, which would be a backwards-incompatible change. A subtle (and fortunately minor) change is that if a string can be parsed without problems, but the `parse_strict` value is invalid, then the updated code raises an error. This is because the new code uses the `parse_strict` value to set up appropriate warning filters, which must be done before calling the unit parsers.
1 parent eb9ab35 commit 8b124d3

7 files changed

Lines changed: 135 additions & 44 deletions

File tree

‎astropy/units/core.py‎

Lines changed: 36 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@
2121
from astropy.utils.misc import isiterable
2222

2323
from . import format as unit_format
24-
from .errors import UnitConversionError, UnitsError, UnitsWarning
24+
from .errors import UnitConversionError, UnitParserWarning, UnitsError, UnitsWarning
2525
from .utils import (
2626
is_effectively_unity,
2727
resolve_fractions,
@@ -60,6 +60,12 @@
6060

6161
UNITY: Final[float] = 1.0
6262

63+
_WARNING_ACTIONS: Final[dict[str, str]] = {
64+
"silent": "ignore",
65+
"warn": "default",
66+
"raise": "error",
67+
}
68+
6369

6470
def _flatten_units_collection(items):
6571
"""
@@ -2099,13 +2105,27 @@ def __call__(
20992105
pass
21002106

21012107
try:
2102-
return f.parse(s)
2108+
with warnings.catch_warnings(
2109+
action=_WARNING_ACTIONS[parse_strict], category=UnitParserWarning
2110+
):
2111+
return f.parse(s)
21032112
except NotImplementedError:
21042113
raise
2114+
except UnitParserWarning as err:
2115+
new_err = ValueError(err)
2116+
new_err.add_note(
2117+
"If you cannot change the unit string then try specifying the "
2118+
"'parse_strict' argument."
2119+
)
2120+
raise new_err from err
2121+
except KeyError as err:
2122+
if parse_strict in _WARNING_ACTIONS:
2123+
raise
2124+
raise ValueError(
2125+
"'parse_strict' must be 'warn', 'raise' or 'silent'"
2126+
) from None
21052127
except Exception as e:
2106-
if parse_strict == "silent":
2107-
pass
2108-
else:
2128+
if parse_strict != "silent":
21092129
# Deliberately not issubclass here. Subclasses
21102130
# should use their name.
21112131
if f is not unit_format.Generic:
@@ -2123,12 +2143,7 @@ def __call__(
21232143
)
21242144
if parse_strict == "raise":
21252145
raise ValueError(msg)
2126-
elif parse_strict == "warn":
2127-
warnings.warn(msg, UnitsWarning)
2128-
else:
2129-
raise ValueError(
2130-
"'parse_strict' must be 'warn', 'raise' or 'silent'"
2131-
)
2146+
warnings.warn(msg, UnitsWarning)
21322147
return UnrecognizedUnit(s)
21332148

21342149
elif isinstance(s, (int, float, np.floating, np.integer)):
@@ -2165,15 +2180,19 @@ class Unit(NamedUnit, metaclass=_UnitMetaClass):
21652180
string is in, by default ``"generic"``. For a description of
21662181
the available formats, see `astropy.units.format`.
21672182
2168-
The optional ``parse_strict`` keyword controls what happens when an
2169-
unrecognized unit string is passed in. It may be one of the following:
2183+
The optional ``parse_strict`` keyword argument controls what happens
2184+
when the string does not comply with the specified format. It may be
2185+
one of the following:
2186+
2187+
- ``'raise'``: (default) raise a `ValueError` exception.
21702188
2171-
- ``'raise'``: (default) raise a ValueError exception.
2189+
- ``'warn'``: emit a `UnitParserWarning`, and return a unit.
21722190
2173-
- ``'warn'``: emit a Warning, and return an
2174-
`UnrecognizedUnit` instance.
2191+
- ``'silent'``: return a unit silently.
21752192
2176-
- ``'silent'``: return an `UnrecognizedUnit` instance.
2193+
With ``'warn'`` or ``'silent'`` the parser might be able to parse the
2194+
string and return a normal unit, but if it fails then an
2195+
`UnrecognizedUnit` instance is returned.
21772196
21782197
- From a number::
21792198

‎astropy/units/errors.py‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
"UnitScaleError",
99
"UnitTypeError",
1010
"UnitsWarning",
11+
"UnitParserWarning",
1112
]
1213

1314
from astropy.utils.exceptions import AstropyWarning
@@ -46,3 +47,7 @@ class UnitsWarning(AstropyWarning):
4647
"""
4748
The base class for unit-specific warnings.
4849
"""
50+
51+
52+
class UnitParserWarning(AstropyWarning):
53+
"""Unit parser warnings"""

‎astropy/units/format/ogip.py‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
from fractions import Fraction
2424
from typing import TYPE_CHECKING
2525

26-
from astropy.units.errors import UnitsWarning
26+
from astropy.units.errors import UnitParserWarning, UnitsWarning
2727
from astropy.utils import classproperty, parsing
2828

2929
from . import core, generic, utils
@@ -216,13 +216,19 @@ def p_unit_expression(p):
216216
)
217217

218218
if len(p) == 7:
219-
raise ValueError(
220-
bad_multiplication_message.format(p[1], f"({p[3]})**{p[6]}")
219+
warnings.warn(
220+
bad_multiplication_message.format(p[1], f"({p[3]})**{p[6]}"),
221+
UnitParserWarning,
221222
)
223+
p[0] = p[1] * p[3] ** p[6]
222224
elif len(p) == 6:
223225
p[0] = p[2] ** p[5]
224226
elif len(p) == 5:
225-
raise ValueError(bad_multiplication_message.format(p[1], f"({p[3]})"))
227+
warnings.warn(
228+
bad_multiplication_message.format(p[1], f"({p[3]})"),
229+
UnitParserWarning,
230+
)
231+
p[0] = p[1] * p[3]
226232
elif len(p) == 4:
227233
p[0] = p[2]
228234
else:

‎astropy/units/tests/test_format.py‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,15 @@
1717

1818
from astropy import units as u
1919
from astropy.constants import si
20-
from astropy.units import PrefixUnit, Unit, UnitBase, UnitsWarning, cds, dex
20+
from astropy.units import (
21+
PrefixUnit,
22+
Unit,
23+
UnitBase,
24+
UnitParserWarning,
25+
UnitsWarning,
26+
cds,
27+
dex,
28+
)
2129
from astropy.units import format as u_format
2230
from astropy.units.utils import is_effectively_unity
2331
from astropy.utils.exceptions import AstropyDeprecationWarning
@@ -279,14 +287,15 @@ def test_ogip_sqrt(string):
279287

280288

281289
@pytest.mark.parametrize(
282-
"string,message",
290+
"string,message,unit",
283291
[
284292
pytest.param(
285293
"m(s)**2",
286294
(
287295
r"^if 'm\(s\)\*\*2' was meant to be a multiplication, "
288296
r"it should have been written as 'm \(s\)\*\*2'.$"
289297
),
298+
u.m * u.s**2,
290299
id="m(s)**2",
291300
),
292301
pytest.param(
@@ -295,14 +304,15 @@ def test_ogip_sqrt(string):
295304
r"^if 'm\(s\)' was meant to be a multiplication, "
296305
r"it should have been written as 'm \(s\)'.$"
297306
),
307+
u.m * u.s,
298308
id="m(s)",
299309
),
300310
],
301311
)
302-
def test_ogip_invalid_multiplication(string, message):
312+
def test_ogip_invalid_multiplication(string, message, unit):
303313
# Regression test for #16749
304-
with pytest.raises(ValueError, match=message):
305-
u_format.OGIP.parse(string)
314+
with pytest.warns(UnitParserWarning, match=message):
315+
assert u_format.OGIP.parse(string) == unit
306316

307317

308318
class RoundtripBase:

‎astropy/units/tests/test_units.py‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
import operator
55
import pickle
6+
from contextlib import nullcontext
67
from fractions import Fraction
78

89
import numpy as np
@@ -229,6 +230,48 @@ def test_unknown_unit3():
229230
u.Unit(None)
230231

231232

233+
@pytest.mark.parametrize(
234+
"parse_strict,expectation",
235+
[
236+
pytest.param("silent", nullcontext(), id="silent"),
237+
pytest.param(
238+
"warn",
239+
pytest.warns(u.UnitParserWarning, match=r"meant to be a multiplication,"),
240+
id="warn",
241+
),
242+
pytest.param(
243+
"raise",
244+
pytest.raises(ValueError, match=r"was meant to be a multiplication, "),
245+
id="raise",
246+
),
247+
pytest.param(
248+
"error",
249+
pytest.raises(
250+
ValueError,
251+
match=r"^'parse_strict' must be 'warn', 'raise' or 'silent'$",
252+
),
253+
id="invalid",
254+
),
255+
],
256+
)
257+
def test_parse_strict_noncritical_error(parse_strict, expectation):
258+
with expectation:
259+
assert u.Unit("m(s)", format="ogip", parse_strict=parse_strict) == u.m * u.s
260+
261+
262+
def test_parse_strict_noncritical_error_default():
263+
with pytest.raises(
264+
ValueError,
265+
match=(
266+
r"^if 'm\(s\)' was meant to be a multiplication, it should have been "
267+
r"written as 'm \(s\)'\.\n"
268+
"If you cannot change the unit string then try specifying the "
269+
r"'parse_strict' argument\.$"
270+
),
271+
):
272+
assert u.Unit("m(s)", format="ogip")
273+
274+
232275
def test_invalid_scale():
233276
with pytest.raises(TypeError):
234277
["a", "b", "c"] * u.m

‎docs/changes/units/16892.feature.rst‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
When parsing invalid unit strings with ``u.Unit(..., parse_strict="warn")`` or
2+
``u.Unit(..., parse_strict="silent")``, a normal unit may be returned if the
3+
problem is not too serious.
4+
If parsing the string fails completely then an ``UnrecognizedUnit`` instance is
5+
returned, just as before.

‎docs/units/format.rst‎

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -245,12 +245,10 @@ following formats:
245245
Dealing with Unrecognized Units
246246
===============================
247247

248-
Since many files found in the wild have unit strings that do not
249-
correspond to any given standard, `astropy.units` also has a
250-
consistent way to store and pass around unit strings that did not
251-
parse. In addition, it provides tools for transforming non-standard,
252-
legacy or misspelt unit strings into their standardized form,
253-
preventing the further propagation of these unit strings.
248+
Since many files found in the wild have unit strings that do not correspond to
249+
any given standard, :mod:`astropy.units` contains functionality for both
250+
validating the strings, and for reading in datasets that contain invalid unit
251+
strings.
254252

255253
By default, passing an unrecognized unit string raises an exception::
256254

@@ -265,25 +263,30 @@ By default, passing an unrecognized unit string raises an exception::
265263
code, enable it with 'u.add_enabled_units'. For details, see
266264
https://docs.astropy.org/en/latest/units/combining_and_defining.html
267265

268-
However, the `~astropy.units.Unit` constructor has the keyword
266+
However, the :class:`~astropy.units.Unit` constructor obeys the keyword
269267
argument ``parse_strict`` that can take one of three values to control
270268
this behavior:
271269

272270
- ``'raise'``: (default) raise a :class:`ValueError`.
273271

274-
- ``'warn'``: emit a :class:`~astropy.units.UnitsWarning`, and return an
275-
`~astropy.units.UnrecognizedUnit` instance.
276-
277-
- ``'silent'``: return an `~astropy.units.UnrecognizedUnit`
278-
instance.
279-
280-
By either adding additional unit aliases for the misspelt units with
272+
- ``'warn'``: emit a :class:`~astropy.units.UnitParserWarning`, and return a
273+
unit.
274+
275+
- ``'silent'``: return a unit without raising errors or emitting warnings.
276+
277+
The type of unit returned when ``parse_strict`` is ``'warn'`` or ``'silent'``
278+
depends on how serious the standard violation is.
279+
In case of a minor standard violation, :class:`~astropy.units.Unit` can
280+
sometimes nonetheless parse the unit.
281+
The warning message (if ``parse_strict='warn'``) will then contain information
282+
about how the invalid string was interpreted.
283+
In case of more serious standard violations an
284+
:class:`~astropy.units.UnrecognizedUnit` instance is returned instead.
285+
In such cases, particularly in case of misspelt units, it might be helpful to
286+
register additional unit aliases with
281287
:func:`~astropy.units.set_enabled_aliases` (e.g., 'Angstroms' for 'Angstrom';
282-
as demonstrated below), or defining new units via
288+
as demonstrated below), or to define new units via
283289
:func:`~astropy.units.def_unit` and :func:`~astropy.units.add_enabled_units`,
284-
we can use ``parse_strict='raise'`` to rapidly find issues with the units used,
285-
while also being able to read in older datasets where the unit usage may have
286-
been less standard.
287290

288291

289292
Examples

0 commit comments

Comments
 (0)