fix: broken docstring examples in Twist2/Twist3/DualQuaternion (and their shared root cause) - #225
Open
petercorke wants to merge 2 commits into
Open
petercorke wants to merge 2 commits into
petercorke wants to merge 2 commits into
Conversation
…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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.whad aS.wtypo, and the "prismatic" branch passed a wrong-shape zero argument forTwist2(3-element list where a scalar is required).Twist2.pole -- the docstring's own example called
S.pole(), butpoleis a@property, not a method.DualQuaternion: a shared root cause across three methods -- fixing
SE3()'s docstring (calledd.Tinstead of the actuald.SE3()method) surfaced thatUnitQuaternion.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 satisfyq*conj(q) == 1. FixedQuaternion.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.dq * v): unrelated to the sign bug -- affected every case regardless of sign. The textbookq*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-correctSE3()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 forconj()'s sign behaviour, both branches ofTwist2/Twist3.unit(),Twist2/Twist3.pole, andDualQuaternion'sSE3()/norm()/vector-transform.Test plan