Skip to content

Commit 4166890

Browse files
petercorketaughz
authored andcommitted
fix(quaternion): make __imatmul__ actually normalize
Quaternion.__imatmul__'s own docstring claimed `q1 @= q2` sets `q1 := qnorm(q1 * q2)`, but the implementation just delegated to __mul__ - identical to plain *=, no normalization at all, contradicting both the docstring and the entire point of adding a separate @= operator. UnitQuaternion.__matmul__ (pre-existing, unchanged) already does this correctly via smb.qunit(smb.qqmul(x, y)) - qunit being the normalizer; qnorm just returns the scalar magnitude, so it was never actually the right function despite the docstring's wording. Fixed by having __imatmul__ delegate to `left @ right` instead of left.__mul__(right). Deliberately not left.__matmul__(right): plain Quaternion has no __matmul__ (only UnitQuaternion defines one, with normalization), and calling the dunder directly as a plain attribute bypasses Python's normal operator fallback, raising a confusing AttributeError instead of the same TypeError `q1 @ q2` already raises for plain Quaternion. `left @ right` matches @'s behaviour exactly in both cases: normalizes for UnitQuaternion, raises consistently for Quaternion. Also fixed the docstring's `-> bool` return type (should be `-> Quaternion`) and its example, which used Quaternion.Eul() - a method that only exists on UnitQuaternion. Tests: added @= coverage for UnitQuaternion (must match @, not *) and for plain Quaternion (must raise TypeError, matching @, not silently degrade to *=).
1 parent 9f1a49d commit 4166890

3 files changed

Lines changed: 39 additions & 6 deletions

File tree

spatialmath/quaternion.py

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -670,29 +670,37 @@ def __imul__(
670670

671671
def __imatmul__(
672672
left, right: Quaternion
673-
) -> bool: # lgtm[py/not-named-self] pylint: disable=no-self-argument
673+
) -> Quaternion: # lgtm[py/not-named-self] pylint: disable=no-self-argument
674674
"""
675675
Overloaded ``@=`` operator
676676
677677
:return: product
678678
:rtype: Quaternion
679679
:raises: ValueError
680680
681-
``q1 @= q2`` sets ``q1 := qnorm(q1 * q2`)`
681+
``q1 @= q2`` sets ``q1 := qnorm(q1 * q2)``. Only meaningful for
682+
``UnitQuaternion``, which is the only subclass defining ``__matmul__``
683+
(with normalization) that this delegates to; on a plain ``Quaternion``
684+
this raises the same ``TypeError`` that ``q1 @ q2`` would.
682685
683686
Example:
684687
685688
.. runblock:: pycon
686689
687-
>>> from spatialmath import Quaternion
690+
>>> from spatialmath import UnitQuaternion
688691
>>> q = UnitQuaternion.Eul([0.1, 0.2, 0.3])
689-
>>> q @= Quaternion.Eul([0.3, 0.4, 0.5])
692+
>>> q @= UnitQuaternion.Eul([0.3, 0.4, 0.5])
690693
>>> print(q)
691694
692695
693-
:seealso: :func:`__mul__`
696+
:seealso: :func:`__matmul__`
694697
"""
695-
return left.__mul__(right)
698+
# NOT left.__matmul__(right): Quaternion itself has no __matmul__
699+
# (only UnitQuaternion defines one), and calling the dunder
700+
# directly as a plain attribute skips Python's normal operator
701+
# fallback, raising a confusing AttributeError instead of the
702+
# TypeError that `left @ right` raises consistently.
703+
return left @ right
696704

697705
def __pow__(self, n: int) -> Quaternion:
698706
"""

tests/test_pose3d.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,11 @@ def test_arith(self):
419419
self.assertIsInstance(R, SO3)
420420
array_compare(R, rotx(pi / 2))
421421

422+
R = SO3()
423+
R @= SO3.Rx(pi / 2)
424+
self.assertIsInstance(R, SO3)
425+
array_compare(R, rotx(pi / 2))
426+
422427
R = SO3()
423428
R *= 2
424429
self.assertNotIsInstance(R, SO3)
@@ -1078,6 +1083,13 @@ def test_arith(self):
10781083
T, np.array([[0, 0, 1, 1], [0, 1, 0, 2], [-1, 0, 0, 3], [0, 0, 0, 1]])
10791084
)
10801085

1086+
T = SE3(1, 2, 3)
1087+
T @= SE3.Ry(pi / 2)
1088+
self.assertIsInstance(T, SE3)
1089+
array_compare(
1090+
T, np.array([[0, 0, 1, 1], [0, 1, 0, 2], [-1, 0, 0, 3], [0, 0, 0, 1]])
1091+
)
1092+
10811093
T = SE3()
10821094
T *= 2
10831095
self.assertNotIsInstance(T, SE3)

tests/test_quaternion.py

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -507,6 +507,11 @@ def test_matmul(self):
507507
UnitQuaternion([ry * rx, rz * ry, rx * rz]),
508508
)
509509

510+
# @= is @ as an augmented assignment, not *=
511+
q = rx
512+
q @= ry
513+
qcompare(q, rx @ ry)
514+
510515
# def multiply_test_normalized(self):
511516

512517
# vx = [1, 0, 0]; vy = [0, 1, 0]; vz = [0, 0, 1]
@@ -949,6 +954,14 @@ def test_multiply(self):
949954
q *= q2
950955
qcompare(q, [-12, 6, 24, 12])
951956

957+
# plain Quaternion has no @ (only UnitQuaternion normalizes via @),
958+
# so @= must fail the same way @ does, not silently fall back to *=
959+
with self.assertRaises(TypeError):
960+
q1 @ q2
961+
with self.assertRaises(TypeError):
962+
q = q1
963+
q @= q2
964+
952965
# vector x vector
953966
qcompare(
954967
Quaternion([q1, u, q2, u, q3, u]) * Quaternion([u, q1, u, q2, u, q3]),

0 commit comments

Comments
 (0)