Skip to content

fix: broken docstring examples in Twist2/Twist3/DualQuaternion (and their shared root cause) - #225

Open
petercorke wants to merge 2 commits into
masterfrom
fix/broken-docstring-examples
Open

petercorke wants to merge 2 commits into
masterfrom
fix/broken-docstring-examples

Conversation

@petercorke

Copy link
Copy Markdown
Collaborator

Summary

Found via the sphinx-pyrunblock rollout (#221): several .. runblock:: docstring examples were silently baking broken tracebacks into the published docs instead of demonstrating real output.

Twist2.unit() / Twist3.unit() -- branches were swapped, self.w had a S.w typo, and the "prismatic" branch passed a wrong-shape zero argument for Twist2 (3-element list where a scalar is required).

Twist2.pole -- the docstring's own example called S.pole(), but pole is a @property, not a method.

DualQuaternion: a shared root cause across three methods -- fixing SE3()'s docstring (called d.T instead of the actual d.SE3() method) surfaced that UnitQuaternion.conj() silently re-canonicalized its result's sign, which is correct for constructing a UnitQuaternion (q and -q are the same rotation) but wrong for conjugation, an algebraic operation that must satisfy q*conj(q) == 1. Fixed Quaternion.conj() itself rather than routing around it at each call site. This one fix resolved three separate, previously-untested bugs:

  • SE3(): silently returned a transform with negated translation for any rotation with negative quaternion scalar part.
  • norm(): crashed outright (math domain error) for the same reason -- also added a small floating-point clamp for the mathematically-non-negative-but-can-round-negative norm-squared terms.
  • Vector transformation (dq * v): unrelated to the sign bug -- affected every case regardless of sign. The textbook q*P*conj(q) sandwich product's translation terms cancel to exactly zero under this class's own dual-part embedding convention (dual = 0.5*Pure(t)*real). Reused the already-correct SE3() extraction instead of hand-deriving a second, convention-specific sandwich formula.

Test coverage

All of the above had zero prior test coverage -- existing DualQuaternion tests used SE3.Rx(pi/4) (no translation, positive quaternion scalar part), which can't exercise any of these bugs. Added regression tests for conj()'s sign behaviour, both branches of Twist2/Twist3.unit(), Twist2/Twist3.pole, and DualQuaternion's SE3()/norm()/vector-transform.

Test plan

  • Full suite: 334 passed (up from 326), 0 regressions
  • Real sphinx-build verification (isolated venv, before/after this branch): RUNBLOCK-ERROR count 6 -> 0, warning count unchanged at 14 (confirmed stable across repeat builds of both branches, ruling out build-to-build nondeterminism as an explanation either way)

…he root cause they shared

Found via the sphinx-pyrunblock rollout (PR #221): several `.. runblock::`
docstring examples were silently baking broken tracebacks into the
published docs instead of demonstrating real output.

## Twist2.unit() / Twist3.unit()

Branches were swapped (comments didn't match what the code under them
did), `self.w` had a `S.w` typo (undefined name), and the "prismatic"
branch passed a wrong-shape zero argument for Twist2 (3-element list
where a scalar is required). Fixed the branch logic, and use `abs()`
instead of `smb.norm()` for Twist2's scalar `w`.

## Twist2.pole

The docstring's own example called `S.pole()`, but `pole` is a
`@property`, not a method -- `TypeError: 'numpy.ndarray' object is not
callable`. (Twist3.pole's own example was already correct; only its
prose incorrectly said `X.pole()` too.)

## DualQuaternion: a shared root cause across three methods

Fixing `DualQuaternion.SE3()`'s docstring (called `d.T` instead of the
actual `d.SE3()` method) surfaced a real bug: `UnitQuaternion.conj()`
silently re-canonicalized its result's sign (forces scalar part >= 0),
which is correct for constructing a `UnitQuaternion` from arbitrary data
(q and -q are the same rotation) but wrong for conjugation, an algebraic
operation that must satisfy q*conj(q) == 1 for downstream algebra to be
correct.

Fixed `Quaternion.conj()` itself (spatialmath/quaternion.py) rather than
routing around it at each call site -- this is what "conjugate" should
mean regardless of caller. No existing test asserted the old
(re-canonicalizing) behaviour, only the return type.

This one fix resolved three separate, previously-untested DualQuaternion
bugs at once:
- `SE3()`: silently returned a transform with negated translation for
  any rotation with negative quaternion scalar part.
- `norm()`: crashed outright (`math domain error`) for the same reason
  -- also added a small floating-point clamp, since the norm-squared
  terms are mathematically non-negative but rounding can leave e.g.
  -1e-17 instead of exactly 0.
- Vector transformation (`dq * v`): found while re-verifying `norm()`'s
  fix -- unrelated to the conj() sign bug, this affected *every* case
  regardless of sign. The textbook q*P*conj(q) sandwich product's
  translation terms cancel to exactly zero under this class's own
  dual-part embedding convention (`__init__` builds `dual =
  0.5*Pure(t)*real`, translation on the left) -- silently applying only
  the rotation and dropping translation entirely. Since `SE3()` already
  correctly extracts (R, t) from this same embedding, reused it instead
  of hand-deriving a second, convention-specific sandwich formula.

## Test coverage

All of the above had zero prior test coverage (existing DualQuaternion
tests used SE3.Rx(pi/4) -- no translation, positive quaternion scalar
part -- which can't exercise any of these bugs). Added regression tests
for conj()'s sign behaviour (including a mixed-sign multi-valued case),
both branches of Twist2/Twist3.unit(), Twist2/Twist3.pole, and
DualQuaternion's SE3()/norm()/vector-transform with both a
negative-quaternion-scalar case and a general case.

Full suite: 334 passed (up from 326), 0 regressions. Real sphinx-build
verification (isolated venv, before/after): RUNBLOCK-ERROR count 6 -> 0,
warning count unchanged at 14 (confirmed stable across repeat builds of
both branches, ruling out build-to-build nondeterminism).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant