Skip to content

Commit 74f4e01

Browse files
nstarmanclaude
andauthored
* 🐛 fix(quantity): improve bitwise ops for dimensionful quantities (#791)
Bitwise/logical operations on dimensionful quantities previously leaked an internal `UnitConversionError` from `ustrip`, producing the misleading message that `'m'` and `''` were not convertible even when both operands had the same unit. Require bitwise operands to be dimensionless before dispatching to JAX and raise a clear `ValueError` identifying the operation and the offending units instead. Apply this consistently across all bitwise entry points, including quantity/quantity, quantity/array (both operand orders), and unary `not`. Bitwise operations on genuinely dimensionless quantities are unchanged. Expand the test suite to cover every dispatch path, verify the operation name appears in the error message, and add regression coverage for the dimensionless success cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 9a570a3 commit 74f4e01

2 files changed

Lines changed: 100 additions & 0 deletions

File tree

src/unxt/_src/quantity/register_primitives.py

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,27 @@ def _to_val_rad_or_one(q: ABCQ) -> ArrayLike:
4343
return ustrip(radian if is_unit_convertible(q.unit, radian) else one, q)
4444

4545

46+
def _require_dimensionless_bitwise(op_name: str, /, *operands: Any) -> None:
47+
"""Raise a clear error if a bitwise/logical op gets dimensionful operands.
48+
49+
These ops require dimensionless operands. Without this check ``ustrip(one, x)``
50+
leaks astropy's confusing "<unit> and '' are not convertible" message -- which,
51+
for ``Q(bool, "m") + Q(bool, "m")`` (JAX lowers a bool ``+`` to ``or_p``),
52+
wrongly implies a dimensionless operand the user never wrote.
53+
54+
Guards every bitwise/logical entry point -- the two-quantity, quantity/array,
55+
and unary overloads -- so the error is consistently clear regardless of which
56+
operand carries the unit. A plain array operand (``unit_of`` returns ``None``)
57+
is inherently dimensionless.
58+
"""
59+
units = [unit_of(o) for o in operands]
60+
if all(u is None or u.is_equivalent(one) for u in units):
61+
return
62+
got = " and ".join(repr("" if u is None else str(u)) for u in units)
63+
msg = f"{op_name} requires dimensionless quantities, got units {got}"
64+
raise ValueError(msg)
65+
66+
4667
def _as_dimensionless_like(q: ABCQ, value: ArrayLike) -> ABCQ:
4768
"""Wrap a dimensionless-valued result as a dimensionless quantity like ``q``.
4869
@@ -397,6 +418,7 @@ def and_p_aq(x1: ABCQ, x2: ABCQ, /) -> ABCQ:
397418
Quantity(Array(0, dtype=int32...), unit='')
398419
399420
"""
421+
_require_dimensionless_bitwise("bitwise/logical and", x1, x2)
400422
return _as_dimensionless_like(x1, lax.and_p.bind(ustrip(one, x1), ustrip(one, x2)))
401423

402424

@@ -418,6 +440,7 @@ def and_p_qv(x1: ABCQ, x2: ArrayLike, /) -> ABCQ:
418440
Quantity(Array([ True, False, False], dtype=bool), unit='')
419441
420442
"""
443+
_require_dimensionless_bitwise("bitwise/logical and", x1, x2)
421444
return _as_dimensionless_like(x1, lax.and_p.bind(ustrip(one, x1), x2))
422445

423446

@@ -438,6 +461,7 @@ def and_p_vq(x1: ArrayLike, x2: ABCQ, /) -> ABCQ:
438461
Quantity(Array([ True, False, False], dtype=bool), unit='')
439462
440463
"""
464+
_require_dimensionless_bitwise("bitwise/logical and", x1, x2)
441465
return _as_dimensionless_like(x2, lax.and_p.bind(x1, ustrip(one, x2)))
442466

443467

@@ -3860,6 +3884,7 @@ def not_p(x: ABCQ, /) -> ABCQ:
38603884
Quantity(Array(-2, dtype=int32...), unit='')
38613885
38623886
"""
3887+
_require_dimensionless_bitwise("bitwise/logical not", x)
38633888
return _as_dimensionless_like(x, lax.bitwise_not(ustrip(one, x)))
38643889

38653890

@@ -3885,6 +3910,7 @@ def or_p_qq(x: ABCQ, y: ABCQ, /) -> ABCQ:
38853910
Quantity(Array(3, dtype=int32...), unit='')
38863911
38873912
"""
3913+
_require_dimensionless_bitwise("bitwise/logical or", x, y)
38883914
return _as_dimensionless_like(x, lax.bitwise_or(ustrip(one, x), ustrip(one, y)))
38893915

38903916

@@ -3907,6 +3933,7 @@ def or_p_qv(x: ABCQ, y: ArrayLike, /) -> ABCQ:
39073933
Quantity(Array([ True, False, True], dtype=bool), unit='')
39083934
39093935
"""
3936+
_require_dimensionless_bitwise("bitwise/logical or", x, y)
39103937
return _as_dimensionless_like(x, lax.or_p.bind(ustrip(one, x), y))
39113938

39123939

@@ -3927,6 +3954,7 @@ def or_p_vq(x: ArrayLike, y: ABCQ, /) -> ABCQ:
39273954
Quantity(Array([ True, False, True], dtype=bool), unit='')
39283955
39293956
"""
3957+
_require_dimensionless_bitwise("bitwise/logical or", x, y)
39303958
return _as_dimensionless_like(y, lax.or_p.bind(x, ustrip(one, y)))
39313959

39323960

@@ -5360,6 +5388,7 @@ def xor_p_qq(x: ABCQ, y: ABCQ, /) -> ABCQ:
53605388
Quantity(Array(3, dtype=int32...), unit='')
53615389
53625390
"""
5391+
_require_dimensionless_bitwise("bitwise/logical xor", x, y)
53635392
return _as_dimensionless_like(x, lax.bitwise_xor(ustrip(one, x), ustrip(one, y)))
53645393

53655394

@@ -5382,6 +5411,7 @@ def xor_p_qv(x: ABCQ, y: ArrayLike, /) -> ABCQ:
53825411
Quantity(Array([False, True, True], dtype=bool), unit='')
53835412
53845413
"""
5414+
_require_dimensionless_bitwise("bitwise/logical xor", x, y)
53855415
return _as_dimensionless_like(x, lax.xor_p.bind(ustrip(one, x), y))
53865416

53875417

@@ -5402,6 +5432,7 @@ def xor_p_vq(x: ArrayLike, y: ABCQ, /) -> ABCQ:
54025432
Quantity(Array([False, True, True], dtype=bool), unit='')
54035433
54045434
"""
5435+
_require_dimensionless_bitwise("bitwise/logical xor", x, y)
54055436
return _as_dimensionless_like(y, lax.xor_p.bind(x, ustrip(one, y)))
54065437

54075438

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
"""Bitwise/logical ops on quantities require dimensionless operands.
2+
3+
The error must name the operation and both *actual* units, not leak astropy's
4+
misleading "'m' and '' are not convertible" (which implies a dimensionless
5+
operand the user never wrote).
6+
"""
7+
8+
import jax.numpy as jnp
9+
import pytest
10+
11+
import quaxed.numpy as qnp
12+
13+
import unxt as u
14+
15+
# (callable, op-name as it appears in the error message)
16+
BITWISE_OPS = [
17+
(qnp.bitwise_or, "or"),
18+
(qnp.bitwise_and, "and"),
19+
(qnp.bitwise_xor, "xor"),
20+
]
21+
22+
23+
def test_bool_add_on_dimensionful_gives_clear_error():
24+
"""`Q(bool, 'm') + Q(bool, 'm')` (JAX lowers to or_p) names or and both 'm'."""
25+
q = u.Q(jnp.array([True, False]), "m")
26+
pattern = r"bitwise/logical or.*dimensionless.*'m'.*'m'"
27+
with pytest.raises(ValueError, match=pattern):
28+
_ = q + q
29+
30+
31+
@pytest.mark.parametrize(("op", "name"), BITWISE_OPS)
32+
def test_bitwise_qq_on_dimensionful_raises_clear_error(op, name):
33+
"""Two dimensionful quantities: the message names the op and both units."""
34+
with pytest.raises(
35+
ValueError, match=rf"bitwise/logical {name}.*dimensionless.*'m'.*'s'"
36+
):
37+
_ = op(u.Q(1, "m"), u.Q(2, "s"))
38+
39+
40+
@pytest.mark.parametrize(("op", "name"), BITWISE_OPS)
41+
def test_bitwise_quantity_array_raises_clear_error(op, name):
42+
"""A dimensionful quantity mixed with a plain array is caught just as clearly.
43+
44+
Both operand orders route through the quantity/array overloads, which strip
45+
to dimensionless too and would otherwise leak astropy's confusing message.
46+
"""
47+
pattern = rf"bitwise/logical {name}.*dimensionless.*'m'"
48+
with pytest.raises(ValueError, match=pattern):
49+
_ = op(u.Q(jnp.array([1]), "m"), jnp.array([2]))
50+
with pytest.raises(ValueError, match=pattern):
51+
_ = op(jnp.array([2]), u.Q(jnp.array([1]), "m"))
52+
53+
54+
def test_bitwise_not_on_dimensionful_raises_clear_error():
55+
"""The unary `not_p` overload is guarded as well."""
56+
with pytest.raises(ValueError, match=r"bitwise/logical not.*dimensionless.*'m'"):
57+
_ = qnp.bitwise_not(u.Q(jnp.array([1]), "m"))
58+
59+
60+
@pytest.mark.parametrize(("op", "name"), BITWISE_OPS)
61+
def test_bitwise_on_dimensionless_still_works(op, name):
62+
"""The dimensionless path is unchanged, for both quantity/quantity and array."""
63+
assert op(u.Q(6, ""), u.Q(3, "")).unit == u.unit("")
64+
assert op(u.Q(jnp.array([1]), ""), jnp.array([2])).unit == u.unit("")
65+
assert op(jnp.array([2]), u.Q(jnp.array([1]), "")).unit == u.unit("")
66+
67+
68+
def test_bitwise_not_on_dimensionless_still_works():
69+
assert qnp.bitwise_not(u.Q(1, "")).unit == u.unit("")

0 commit comments

Comments
 (0)