Repository navigation
Fix frame labels in chapter 2 notebook - #50
Merged
petercorke merged 1 commit intoSep 26, 2026
Merged
Conversation
Owner
|
Thanks for this. It led to PRs for |
taughz
pushed a commit
to rai-opensource/spatialmath-python
that referenced
this pull request
Oct 16, 2026
## Summary `trplot()` and `trplot2()` both ended with a bare `**kwargs` that was never read anywhere in their bodies, so a typo'd keyword (e.g. `framelabel=` instead of `frame=`) was silently accepted and ignored instead of raising — it drew the frame with no error and no label at all. This surfaced downstream in [petercorke/RVC3-python#50](petercorke/RVC3-python#50), where a notebook had exactly this typo for years with no test ever catching it. - Removed the dead `**kwargs` sink from both functions so an unrecognized keyword now raises `TypeError`, same as any normal Python call. - `trplot`'s `anaglyph` branch only forwarded a small fixed subset of parameters to each eye's recursive call, silently dropping `textcolor`, `labels`, `originsize`, `origincolor`, `axislabel`, `axissubscript`, `width` and `projection`. It also never returned `ax` and never actually called `plt.show(block=...)`, so anaglyph mode ignored `block` entirely. Fixed to forward the full applicable parameter set, return `ax`, and call `plt.show()` once after both eyes are drawn. - `trplot`'s "T is an iterable of transforms" branch forwarded every parameter except `axissubscript`, and also never returned `ax`. Fixed. - `tranimate`/`tranimate2` were splatting the *same* unfiltered kwargs dict into `Animate()`/`Animate2()`'s constructor, the drawing call, and `run()`'s animation-control call, relying on each one's own dead `**kwargs` sink to drop what it didn't need. Now that `trplot`/`trplot2` validate strictly, `run()`-only parameters (`movie`, `repeat`, `interval`, `nframes`, `wait`) are split off before the drawing call. - Fixed a long-standing typo in `test_pose2d.py::test_graphics` (`T0=T2`, which was never a real parameter — `animate()`'s documented one is `start=`) that this same silent-swallow bug had been quietly masking since the test was written. ## New test coverage Added `tests/base/test_animate.py`. While chasing the `tranimate`/`tranimate2` regression above, I found that constructing a `FuncAnimation` under a non-interactive backend (the Agg backend this repo's whole test suite runs under) doesn't actually run its per-frame `update()` callback — confirmed directly by checking `Animate2`'s internal frame counter, which never advances past its initial value unless something drives the animation. So the existing `animate()`/`tranimate()` tests only ever proved construction didn't raise, not that interpolation/drawing works. The new tests force every frame through the real callback via `FuncAnimation.save()` with `PillowWriter` (pure Python, no external `ffmpeg` dependency) and assert `trinterp`/`trinterp2` actually ran across a real `s: 0→1` sweep. This is what caught the `tranimate`/`tranimate2` kwargs-splitting bug above in the first place. ## Test plan - [x] Full test suite passes: `353 passed, 3 skipped` (skips are pre-existing, display-dependent) - [x] Fail/fix/pass verified for every behavioral change (confirmed each new/modified test fails against the pre-fix code for the intended reason, and passes after) - [x] `SE3`/`SE2` `.plot()`/`.animate()` wrapper methods (which forward `*args, **kwargs` straight through) verified to propagate the stricter validation correctly 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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.
Replace
framelabelwithframein the threetrplot2calls in Chapter 2's rotation example so the RX, XR and XC labels render correctly.Verified the edited cell runs and renders all three labels with SpatialMath 1.1.18. Full notebook suite not run.