Repository navigation
chore: switch docs build to PyPI sphinx-pyrunblock instead of git+https - #221
Merged
Merged
Conversation
sphinx-pyrunblock is Peter's own package (published to PyPI, hard fork of the original WhyNotHugo/sphinx-autorun), reachable at the same content today via git+https://github.com/petercorke/sphinx-autorun.git (an old, pre-rename URL that still redirects). Installing the PyPI wheel directly is ~2-4x faster in CI (no git clone/VCS resolution) and makes pyproject.toml's declared dependency honest -- previously "sphinx-autorun" there would, in isolation, resolve to the unrelated upstream PyPI package of that name rather than Peter's fork. No functional change: both paths resolve to the same sphinx-pyrunblock 1.1.0 release. Verified with a real sphinx-build against a clean venv.
Merged
2 tasks done
17 of 19 tasks
taughz
pushed a commit
that referenced
this pull request
Oct 16, 2026
…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).
taughz
approved these changes
Oct 16, 2026
taughz
pushed a commit
that referenced
this pull request
Oct 16, 2026
…heir shared root cause) (#225) ## 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 - [x] Full suite: 334 passed (up from 326), 0 regressions - [x] 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)
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
2 participants
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
sphinx-pyrunblockis Peter's own package (a hard fork of the originalWhyNotHugo/sphinx-autorun, published properly to PyPI under this name). Today's CI installs it viapip install git+https://github.com/petercorke/sphinx-autorun.git-- an old, pre-rename URL that still works only because GitHub redirects renamed repos. This switches to installing the plain PyPI wheel instead.pyproject.toml'sdocsextra from"sphinx-autorun"to"sphinx-pyrunblock"(the old entry was misleading in isolation -- without the workflow's follow-up git install, it would resolve to the unrelated upstream PyPI package of the same name, not Peter's fork).pip install git+...line fromsphinx.yml, sincepip install .[dev,docs]already installs the right package.conf.py'sextensionslist fromsphinx_autoruntosphinx_pyrunblockto match.No functional change
Both the old git URL and the new PyPI package resolve to the identical
sphinx-pyrunblock==1.1.0release today -- confirmed with a fresh, uncachedpip install git+...and comparing installed package metadata. This is purely a speed/hygiene change.Why
The PyPI install is consistently 2-4x faster in a clean venv (no git clone/VCS metadata resolution):
git+https://...sphinx-autorun.gitsphinx-pyrunblock(PyPI)That's ~9-15s saved on every docs-build CI run (every push to
masterplus every PR).Test plan
pip install ".[dev,docs]", confirmedsphinx-pyrunblock==1.1.0resolves without the git install linesphinx-build -b htmlagainst the rebased branch (includes perf: defer Matplotlib import until something actually plots #198, Fix sphinx automation #162) -- succeeds, 15 warnings, consistent with pre-change baseline