Skip to content

Commit 876813c

Browse files
petercorkeclaude
authored andcommitted
fix: trplot/trplot2 silently swallowed unknown keyword arguments
Both trplot() and trplot2() 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. Removing the dead sink lets Python's own unexpected-keyword-argument check do its job. While rewriting the two places that used to lean on this catch-all: - 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. - trplot's "T is an iterable of transforms" branch forwarded every parameter except axissubscript, and also never returned ax. - 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. Once trplot/trplot2 validate strictly, run()-only parameters (movie, repeat, interval, nframes, wait) needed to be split off before the drawing call. Also fixes 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. Adds tests/base/test_animate.py: constructing a FuncAnimation under a non-interactive backend doesn't actually run its per-frame update() callback (confirmed directly -- Animate2's frame counter never advances past its initial value), so existing animate() tests only proved construction didn't raise. These tests force every frame through the real callback via FuncAnimation.save() (PillowWriter, no external ffmpeg dependency) and assert trinterp/trinterp2 actually ran across a real s: 0->1 sweep. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 57863ee commit 876813c

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)