Skip to content

Commit a6a2f1d

Browse files
petercorkeclaude
andauthored
fix: trplot/trplot2 silently swallowed unknown keyword arguments (#230)
## 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>
1 parent 57863ee commit a6a2f1d

6 files changed

Lines changed: 184 additions & 9 deletions

File tree

‎spatialmath/base/transforms2d.py‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1345,7 +1345,6 @@ def trplot2(
13451345
width: float = 1,
13461346
d1: float = 0.1,
13471347
d2: float = 1.15,
1348-
**kwargs,
13491348
):
13501349
"""
13511350
Plot a 2D coordinate frame
@@ -1612,9 +1611,18 @@ def tranimate2(T: Union[SO2Array, SE2Array], **kwargs):
16121611
"""
16131612
dims = kwargs.pop("dims", None)
16141613
ax = kwargs.pop("ax", None)
1614+
1615+
# Animate2.run()'s own parameters must not also be forwarded to
1616+
# trplot2's drawing call, which now validates its keywords strictly
1617+
# (see Animate2.run's signature for this parameter set)
1618+
run_kwargs = {
1619+
k: kwargs.pop(k)
1620+
for k in ("movie", "repeat", "interval", "nframes", "wait")
1621+
if k in kwargs
1622+
}
16151623
anim = smb.animate.Animate2(dims=dims, axes=ax, **kwargs)
16161624
anim.trplot2(T, **kwargs)
1617-
return anim.run(**kwargs)
1625+
return anim.run(**run_kwargs)
16181626

16191627

16201628
if __name__ == "__main__": # pragma: no cover

‎spatialmath/base/transforms3d.py‎

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3059,7 +3059,6 @@ def trplot(
30593059
dims: Optional[ArrayLikePure] = None,
30603060
d2: float = 1.15,
30613061
flo: Tuple[float, float, float] = (-0.05, -0.05, -0.05),
3062-
**kwargs,
30633062
):
30643063
"""
30653064
Plot a 3D coordinate frame
@@ -3247,6 +3246,9 @@ def trplot(
32473246
ax.set_proj_type("persp")
32483247

32493248
# collect all the arguments to use for left and right views
3249+
# note: dims is deliberately excluded, it was already applied to
3250+
# ax above; block is deliberately excluded and handled once
3251+
# below, after both eyes are drawn, rather than per-eye
32503252
args = {
32513253
"ax": ax,
32523254
"frame": frame,
@@ -3255,8 +3257,15 @@ def trplot(
32553257
"wtl": wtl,
32563258
"flo": flo,
32573259
"d2": d2,
3260+
"textcolor": textcolor,
3261+
"labels": labels,
3262+
"originsize": originsize,
3263+
"origincolor": origincolor,
3264+
"axislabel": axislabel,
3265+
"axissubscript": axissubscript,
3266+
"width": width,
3267+
"projection": projection,
32583268
}
3259-
args = {**args, **kwargs}
32603269

32613270
# unpack the anaglyph parameters
32623271
shift = 0.1
@@ -3278,7 +3287,11 @@ def trplot(
32783287
T = r2t(cast(SO3Array, T))
32793288
trplot(transl(shift, 0, 0) @ T, color=colors[1], **args)
32803289

3281-
return
3290+
if block is not None:
3291+
import matplotlib.pyplot as plt
3292+
3293+
plt.show(block=block)
3294+
return ax
32823295

32833296
if style == "rviz":
32843297
if originsize is None:
@@ -3330,9 +3343,9 @@ def trplot(
33303343
flo=flo,
33313344
anaglyph=anaglyph,
33323345
axislabel=axislabel,
3333-
**kwargs,
3346+
axissubscript=axissubscript,
33343347
)
3335-
return
3348+
return ax
33363349

33373350
if dims is not None:
33383351
dims = tuple(dims)
@@ -3546,9 +3559,18 @@ def tranimate(T: Union[SO3Array, SE3Array], **kwargs) -> str:
35463559
# alias for backward compatibility with the previous docstring/API
35473560
dim = kwargs.pop("dim", kwargs.pop("dims", None))
35483561
ax = kwargs.pop("ax", None)
3562+
3563+
# Animate.run()'s own parameters must not also be forwarded to
3564+
# trplot's drawing call, which now validates its keywords strictly
3565+
# (see Animate.run's signature for this parameter set)
3566+
run_kwargs = {
3567+
k: kwargs.pop(k)
3568+
for k in ("movie", "repeat", "interval", "nframes", "wait")
3569+
if k in kwargs
3570+
}
35493571
anim = Animate(dim=dim, ax=ax, **kwargs)
35503572
anim.trplot(T, **kwargs)
3551-
return anim.run(**kwargs)
3573+
return anim.run(**run_kwargs)
35523574

35533575

35543576
if __name__ == "__main__": # pragma: no cover

‎tests/base/test_animate.py‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
#!/usr/bin/env python3
2+
# -*- coding: utf-8 -*-
3+
"""
4+
Regression coverage for spatialmath.base.animate.
5+
6+
animate()/tranimate()/tranimate2() build a matplotlib FuncAnimation, but
7+
under a non-interactive backend with nothing pumping a real event loop
8+
(exactly the CI/test environment -- see conftest.py, which forces the Agg
9+
backend for the whole session) its per-frame update() callback never
10+
actually fires. Merely constructing the FuncAnimation, or calling
11+
.animate() and letting the result be garbage collected, proves nothing
12+
about whether the interpolation code (trinterp/trinterp2) runs or is
13+
correct -- it only proves construction didn't raise.
14+
15+
FuncAnimation.save() is different: it synchronously steps through every
16+
frame and invokes the real update() callback for each one, regardless of
17+
backend -- the same thing a real playback or movie export would do. We
18+
drive that here with PillowWriter (pure Python, ships with
19+
matplotlib+Pillow, no external ffmpeg binary) purely to force every frame
20+
through the real callback, and record what it actually computed, rather
21+
than analyzing the saved output itself.
22+
"""
23+
24+
import os
25+
import tempfile
26+
import unittest
27+
from unittest.mock import patch
28+
29+
import matplotlib
30+
31+
matplotlib.use("Agg")
32+
import matplotlib.pyplot as plt
33+
from matplotlib.animation import PillowWriter
34+
35+
import spatialmath.base as smb
36+
from spatialmath import SE2, SE3
37+
38+
39+
def _drive_all_frames(fa):
40+
tmp = tempfile.mktemp(suffix=".gif")
41+
try:
42+
fa.save(tmp, writer=PillowWriter(fps=5))
43+
finally:
44+
if os.path.exists(tmp):
45+
os.remove(tmp)
46+
47+
48+
class TestAnimate2(unittest.TestCase):
49+
def test_drives_real_frames(self):
50+
plt.close("all")
51+
end = SE2.Rand()
52+
start = SE2.Rand()
53+
nframes = 5
54+
55+
real_trinterp2 = smb.trinterp2
56+
calls = []
57+
58+
def recording_trinterp2(*args, **kwargs):
59+
calls.append(kwargs["s"])
60+
return real_trinterp2(*args, **kwargs)
61+
62+
with patch("spatialmath.base.trinterp2", side_effect=recording_trinterp2):
63+
fa = end.animate(start=start, dims=[-2, 2], nframes=nframes, repeat=False)
64+
_drive_all_frames(fa)
65+
66+
self.assertEqual(len(calls), nframes)
67+
self.assertAlmostEqual(min(calls), 0.0)
68+
self.assertAlmostEqual(max(calls), 1.0)
69+
plt.close("all")
70+
71+
72+
class TestAnimate3(unittest.TestCase):
73+
def test_drives_real_frames(self):
74+
plt.close("all")
75+
end = SE3.Rand()
76+
start = SE3.Rand()
77+
nframes = 5
78+
79+
real_trinterp = smb.trinterp
80+
calls = []
81+
82+
def recording_trinterp(*args, **kwargs):
83+
calls.append(kwargs["s"])
84+
return real_trinterp(*args, **kwargs)
85+
86+
with patch("spatialmath.base.trinterp", side_effect=recording_trinterp):
87+
fa = end.animate(start=start, dims=[-2, 2], nframes=nframes, repeat=False)
88+
_drive_all_frames(fa)
89+
90+
self.assertEqual(len(calls), nframes)
91+
self.assertAlmostEqual(min(calls), 0.0)
92+
self.assertAlmostEqual(max(calls), 1.0)
93+
plt.close("all")
94+
95+
96+
if __name__ == "__main__":
97+
unittest.main()

‎tests/base/test_transforms2d.py‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,14 @@ def test_plot(self):
312312
)
313313
plt.close("all")
314314

315+
def test_plot_rejects_unknown_kwarg(self):
316+
# trplot2() used to silently swallow an unrecognized keyword via a
317+
# bare **kwargs sink instead of raising -- e.g. a typo'd
318+
# framelabel= (the real parameter is frame=) drew the frame with
319+
# no label and no error at all.
320+
with self.assertRaises(TypeError):
321+
trplot2(transl2(1, 2), framelabel="A", block=False)
322+
315323

316324
# ---------------------------------------------------------------------------------------#
317325
if __name__ == "__main__":

‎tests/base/test_transforms3d_plot.py‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,46 @@ def test_plot(self):
7373

7474
plt.close("all")
7575

76+
def test_plot_rejects_unknown_kwarg(self):
77+
# trplot() used to silently swallow an unrecognized keyword via a
78+
# bare **kwargs sink instead of raising -- e.g. a typo'd
79+
# framelabel= (the real parameter is frame=) drew the frame with
80+
# no label and no error at all.
81+
with self.assertRaises(TypeError):
82+
trplot(transl(1, 2, 3), framelabel="A", block=False)
83+
84+
def test_plot_anaglyph_forwards_params(self):
85+
# the anaglyph branch recurses into trplot() for each eye; it used
86+
# to forward only a small fixed subset of parameters (ax, frame,
87+
# length, style, wtl, flo, d2), silently dropping textcolor,
88+
# labels, originsize, origincolor, axislabel, axissubscript, width
89+
# and projection for both eyes.
90+
plt.figure()
91+
ax = trplot(
92+
transl(1, 2, 3),
93+
frame="A",
94+
anaglyph=True,
95+
block=False,
96+
textcolor="k",
97+
originsize=0,
98+
)
99+
# frame label + 3 axis labels, per eye
100+
self.assertEqual(len(ax.texts), 8)
101+
self.assertTrue(all(t.get_color() == "k" for t in ax.texts))
102+
plt.close("all")
103+
104+
def test_plot_iterable_forwards_axissubscript(self):
105+
# the "T is an iterable of transforms" branch used to forward
106+
# every named parameter except axissubscript when recursing per
107+
# transform, so axissubscript=False on the outer call was ignored.
108+
plt.figure()
109+
T = [transl(1, 2, 3), transl(2, 3, 4)]
110+
ax = trplot(T, frame="F", axissubscript=False, block=False)
111+
axis_labels = [t.get_text() for t in ax.texts if t.get_text() != r"$\{F\}$"]
112+
self.assertTrue(axis_labels)
113+
self.assertTrue(all("_{" not in text for text in axis_labels))
114+
plt.close("all")
115+
76116
@pytest.mark.skipif(
77117
plt.get_backend().lower() == "agg"
78118
or os.environ.get("CI") == "true"

‎tests/test_pose2d.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -509,7 +509,7 @@ def test_graphics(self):
509509
T1.plot(block=False, dims=[-2, 2])
510510

511511
T1.animate(repeat=False, dims=[-2, 2], nframes=10)
512-
T1.animate(T0=T2, repeat=False, dims=[-2, 2], nframes=10)
512+
T1.animate(start=T2, repeat=False, dims=[-2, 2], nframes=10)
513513

514514

515515
# ---------------------------------------------------------------------------------------#

0 commit comments

Comments
 (0)