Skip to content

Commit 2c46775

Browse files
petercorkeclaude
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 2de7801 commit 2c46775

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
@@ -1336,7 +1336,6 @@ def trplot2(
13361336
width: float = 1,
13371337
d1: float = 0.1,
13381338
d2: float = 1.15,
1339-
**kwargs,
13401339
):
13411340
"""
13421341
Plot a 2D coordinate frame
@@ -1603,9 +1602,18 @@ def tranimate2(T: Union[SO2Array, SE2Array], **kwargs):
16031602
"""
16041603
dims = kwargs.pop("dims", None)
16051604
ax = kwargs.pop("ax", None)
1605+
1606+
# Animate2.run()'s own parameters must not also be forwarded to
1607+
# trplot2's drawing call, which now validates its keywords strictly
1608+
# (see Animate2.run's signature for this parameter set)
1609+
run_kwargs = {
1610+
k: kwargs.pop(k)
1611+
for k in ("movie", "repeat", "interval", "nframes", "wait")
1612+
if k in kwargs
1613+
}
16061614
anim = smb.animate.Animate2(dims=dims, axes=ax, **kwargs)
16071615
anim.trplot2(T, **kwargs)
1608-
return anim.run(**kwargs)
1616+
return anim.run(**run_kwargs)
16091617

16101618

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

‎spatialmath/base/transforms3d.py‎

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3029,7 +3029,6 @@ def trplot(
30293029
dims: Optional[ArrayLikePure] = None,
30303030
d2: float = 1.15,
30313031
flo: Tuple[float, float, float] = (-0.05, -0.05, -0.05),
3032-
**kwargs,
30333032
):
30343033
"""
30353034
Plot a 3D coordinate frame
@@ -3217,6 +3216,9 @@ def trplot(
32173216
ax.set_proj_type("persp")
32183217

32193218
# collect all the arguments to use for left and right views
3219+
# note: dims is deliberately excluded, it was already applied to
3220+
# ax above; block is deliberately excluded and handled once
3221+
# below, after both eyes are drawn, rather than per-eye
32203222
args = {
32213223
"ax": ax,
32223224
"frame": frame,
@@ -3225,8 +3227,15 @@ def trplot(
32253227
"wtl": wtl,
32263228
"flo": flo,
32273229
"d2": d2,
3230+
"textcolor": textcolor,
3231+
"labels": labels,
3232+
"originsize": originsize,
3233+
"origincolor": origincolor,
3234+
"axislabel": axislabel,
3235+
"axissubscript": axissubscript,
3236+
"width": width,
3237+
"projection": projection,
32283238
}
3229-
args = {**args, **kwargs}
32303239

32313240
# unpack the anaglyph parameters
32323241
shift = 0.1
@@ -3248,7 +3257,11 @@ def trplot(
32483257
T = r2t(cast(SO3Array, T))
32493258
trplot(transl(shift, 0, 0) @ T, color=colors[1], **args)
32503259

3251-
return
3260+
if block is not None:
3261+
import matplotlib.pyplot as plt
3262+
3263+
plt.show(block=block)
3264+
return ax
32523265

32533266
if style == "rviz":
32543267
if originsize is None:
@@ -3300,9 +3313,9 @@ def trplot(
33003313
flo=flo,
33013314
anaglyph=anaglyph,
33023315
axislabel=axislabel,
3303-
**kwargs,
3316+
axissubscript=axissubscript,
33043317
)
3305-
return
3318+
return ax
33063319

33073320
if dims is not None:
33083321
dims = tuple(dims)
@@ -3516,9 +3529,18 @@ def tranimate(T: Union[SO3Array, SE3Array], **kwargs) -> str:
35163529
# alias for backward compatibility with the previous docstring/API
35173530
dim = kwargs.pop("dim", kwargs.pop("dims", None))
35183531
ax = kwargs.pop("ax", None)
3532+
3533+
# Animate.run()'s own parameters must not also be forwarded to
3534+
# trplot's drawing call, which now validates its keywords strictly
3535+
# (see Animate.run's signature for this parameter set)
3536+
run_kwargs = {
3537+
k: kwargs.pop(k)
3538+
for k in ("movie", "repeat", "interval", "nframes", "wait")
3539+
if k in kwargs
3540+
}
35193541
anim = Animate(dim=dim, ax=ax, **kwargs)
35203542
anim.trplot(T, **kwargs)
3521-
return anim.run(**kwargs)
3543+
return anim.run(**run_kwargs)
35223544

35233545

35243546
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
@@ -296,6 +296,14 @@ def test_plot(self):
296296
)
297297
plt.close("all")
298298

299+
def test_plot_rejects_unknown_kwarg(self):
300+
# trplot2() used to silently swallow an unrecognized keyword via a
301+
# bare **kwargs sink instead of raising -- e.g. a typo'd
302+
# framelabel= (the real parameter is frame=) drew the frame with
303+
# no label and no error at all.
304+
with self.assertRaises(TypeError):
305+
trplot2(transl2(1, 2), framelabel="A", block=False)
306+
299307

300308
# ---------------------------------------------------------------------------------------#
301309
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)