From 2c46775a3ab3c5a3f6d8343c1748543e785d95d9 Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Sat, 26 Sep 2026 17:37:34 -0400 Subject: [PATCH] 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 --- spatialmath/base/transforms2d.py | 12 +++- spatialmath/base/transforms3d.py | 34 ++++++++-- tests/base/test_animate.py | 97 ++++++++++++++++++++++++++++ tests/base/test_transforms2d.py | 8 +++ tests/base/test_transforms3d_plot.py | 40 ++++++++++++ tests/test_pose2d.py | 2 +- 6 files changed, 184 insertions(+), 9 deletions(-) create mode 100644 tests/base/test_animate.py diff --git a/spatialmath/base/transforms2d.py b/spatialmath/base/transforms2d.py index e9bc0e70..d09e820d 100644 --- a/spatialmath/base/transforms2d.py +++ b/spatialmath/base/transforms2d.py @@ -1336,7 +1336,6 @@ def trplot2( width: float = 1, d1: float = 0.1, d2: float = 1.15, - **kwargs, ): """ Plot a 2D coordinate frame @@ -1603,9 +1602,18 @@ def tranimate2(T: Union[SO2Array, SE2Array], **kwargs): """ dims = kwargs.pop("dims", None) ax = kwargs.pop("ax", None) + + # Animate2.run()'s own parameters must not also be forwarded to + # trplot2's drawing call, which now validates its keywords strictly + # (see Animate2.run's signature for this parameter set) + run_kwargs = { + k: kwargs.pop(k) + for k in ("movie", "repeat", "interval", "nframes", "wait") + if k in kwargs + } anim = smb.animate.Animate2(dims=dims, axes=ax, **kwargs) anim.trplot2(T, **kwargs) - return anim.run(**kwargs) + return anim.run(**run_kwargs) if __name__ == "__main__": # pragma: no cover diff --git a/spatialmath/base/transforms3d.py b/spatialmath/base/transforms3d.py index 11aba217..8f958e31 100644 --- a/spatialmath/base/transforms3d.py +++ b/spatialmath/base/transforms3d.py @@ -3029,7 +3029,6 @@ def trplot( dims: Optional[ArrayLikePure] = None, d2: float = 1.15, flo: Tuple[float, float, float] = (-0.05, -0.05, -0.05), - **kwargs, ): """ Plot a 3D coordinate frame @@ -3217,6 +3216,9 @@ def trplot( ax.set_proj_type("persp") # collect all the arguments to use for left and right views + # note: dims is deliberately excluded, it was already applied to + # ax above; block is deliberately excluded and handled once + # below, after both eyes are drawn, rather than per-eye args = { "ax": ax, "frame": frame, @@ -3225,8 +3227,15 @@ def trplot( "wtl": wtl, "flo": flo, "d2": d2, + "textcolor": textcolor, + "labels": labels, + "originsize": originsize, + "origincolor": origincolor, + "axislabel": axislabel, + "axissubscript": axissubscript, + "width": width, + "projection": projection, } - args = {**args, **kwargs} # unpack the anaglyph parameters shift = 0.1 @@ -3248,7 +3257,11 @@ def trplot( T = r2t(cast(SO3Array, T)) trplot(transl(shift, 0, 0) @ T, color=colors[1], **args) - return + if block is not None: + import matplotlib.pyplot as plt + + plt.show(block=block) + return ax if style == "rviz": if originsize is None: @@ -3300,9 +3313,9 @@ def trplot( flo=flo, anaglyph=anaglyph, axislabel=axislabel, - **kwargs, + axissubscript=axissubscript, ) - return + return ax if dims is not None: dims = tuple(dims) @@ -3516,9 +3529,18 @@ def tranimate(T: Union[SO3Array, SE3Array], **kwargs) -> str: # alias for backward compatibility with the previous docstring/API dim = kwargs.pop("dim", kwargs.pop("dims", None)) ax = kwargs.pop("ax", None) + + # Animate.run()'s own parameters must not also be forwarded to + # trplot's drawing call, which now validates its keywords strictly + # (see Animate.run's signature for this parameter set) + run_kwargs = { + k: kwargs.pop(k) + for k in ("movie", "repeat", "interval", "nframes", "wait") + if k in kwargs + } anim = Animate(dim=dim, ax=ax, **kwargs) anim.trplot(T, **kwargs) - return anim.run(**kwargs) + return anim.run(**run_kwargs) if __name__ == "__main__": # pragma: no cover diff --git a/tests/base/test_animate.py b/tests/base/test_animate.py new file mode 100644 index 00000000..147a09ad --- /dev/null +++ b/tests/base/test_animate.py @@ -0,0 +1,97 @@ +#!/usr/bin/env python3 +# -*- coding: utf-8 -*- +""" +Regression coverage for spatialmath.base.animate. + +animate()/tranimate()/tranimate2() build a matplotlib FuncAnimation, but +under a non-interactive backend with nothing pumping a real event loop +(exactly the CI/test environment -- see conftest.py, which forces the Agg +backend for the whole session) its per-frame update() callback never +actually fires. Merely constructing the FuncAnimation, or calling +.animate() and letting the result be garbage collected, proves nothing +about whether the interpolation code (trinterp/trinterp2) runs or is +correct -- it only proves construction didn't raise. + +FuncAnimation.save() is different: it synchronously steps through every +frame and invokes the real update() callback for each one, regardless of +backend -- the same thing a real playback or movie export would do. We +drive that here with PillowWriter (pure Python, ships with +matplotlib+Pillow, no external ffmpeg binary) purely to force every frame +through the real callback, and record what it actually computed, rather +than analyzing the saved output itself. +""" + +import os +import tempfile +import unittest +from unittest.mock import patch + +import matplotlib + +matplotlib.use("Agg") +import matplotlib.pyplot as plt +from matplotlib.animation import PillowWriter + +import spatialmath.base as smb +from spatialmath import SE2, SE3 + + +def _drive_all_frames(fa): + tmp = tempfile.mktemp(suffix=".gif") + try: + fa.save(tmp, writer=PillowWriter(fps=5)) + finally: + if os.path.exists(tmp): + os.remove(tmp) + + +class TestAnimate2(unittest.TestCase): + def test_drives_real_frames(self): + plt.close("all") + end = SE2.Rand() + start = SE2.Rand() + nframes = 5 + + real_trinterp2 = smb.trinterp2 + calls = [] + + def recording_trinterp2(*args, **kwargs): + calls.append(kwargs["s"]) + return real_trinterp2(*args, **kwargs) + + with patch("spatialmath.base.trinterp2", side_effect=recording_trinterp2): + fa = end.animate(start=start, dims=[-2, 2], nframes=nframes, repeat=False) + _drive_all_frames(fa) + + self.assertEqual(len(calls), nframes) + self.assertAlmostEqual(min(calls), 0.0) + self.assertAlmostEqual(max(calls), 1.0) + plt.close("all") + + +class TestAnimate3(unittest.TestCase): + def test_drives_real_frames(self): + plt.close("all") + end = SE3.Rand() + start = SE3.Rand() + nframes = 5 + + real_trinterp = smb.trinterp + calls = [] + + def recording_trinterp(*args, **kwargs): + calls.append(kwargs["s"]) + return real_trinterp(*args, **kwargs) + + with patch("spatialmath.base.trinterp", side_effect=recording_trinterp): + fa = end.animate(start=start, dims=[-2, 2], nframes=nframes, repeat=False) + _drive_all_frames(fa) + + self.assertEqual(len(calls), nframes) + self.assertAlmostEqual(min(calls), 0.0) + self.assertAlmostEqual(max(calls), 1.0) + plt.close("all") + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/base/test_transforms2d.py b/tests/base/test_transforms2d.py index 6a406006..61e3adab 100755 --- a/tests/base/test_transforms2d.py +++ b/tests/base/test_transforms2d.py @@ -296,6 +296,14 @@ def test_plot(self): ) plt.close("all") + def test_plot_rejects_unknown_kwarg(self): + # trplot2() used to silently swallow an unrecognized keyword via a + # bare **kwargs sink instead of raising -- e.g. a typo'd + # framelabel= (the real parameter is frame=) drew the frame with + # no label and no error at all. + with self.assertRaises(TypeError): + trplot2(transl2(1, 2), framelabel="A", block=False) + # ---------------------------------------------------------------------------------------# if __name__ == "__main__": diff --git a/tests/base/test_transforms3d_plot.py b/tests/base/test_transforms3d_plot.py index d18d9300..2a4ba18e 100755 --- a/tests/base/test_transforms3d_plot.py +++ b/tests/base/test_transforms3d_plot.py @@ -73,6 +73,46 @@ def test_plot(self): plt.close("all") + def test_plot_rejects_unknown_kwarg(self): + # trplot() used to silently swallow an unrecognized keyword via a + # bare **kwargs sink instead of raising -- e.g. a typo'd + # framelabel= (the real parameter is frame=) drew the frame with + # no label and no error at all. + with self.assertRaises(TypeError): + trplot(transl(1, 2, 3), framelabel="A", block=False) + + def test_plot_anaglyph_forwards_params(self): + # the anaglyph branch recurses into trplot() for each eye; it used + # to forward only a small fixed subset of parameters (ax, frame, + # length, style, wtl, flo, d2), silently dropping textcolor, + # labels, originsize, origincolor, axislabel, axissubscript, width + # and projection for both eyes. + plt.figure() + ax = trplot( + transl(1, 2, 3), + frame="A", + anaglyph=True, + block=False, + textcolor="k", + originsize=0, + ) + # frame label + 3 axis labels, per eye + self.assertEqual(len(ax.texts), 8) + self.assertTrue(all(t.get_color() == "k" for t in ax.texts)) + plt.close("all") + + def test_plot_iterable_forwards_axissubscript(self): + # the "T is an iterable of transforms" branch used to forward + # every named parameter except axissubscript when recursing per + # transform, so axissubscript=False on the outer call was ignored. + plt.figure() + T = [transl(1, 2, 3), transl(2, 3, 4)] + ax = trplot(T, frame="F", axissubscript=False, block=False) + axis_labels = [t.get_text() for t in ax.texts if t.get_text() != r"$\{F\}$"] + self.assertTrue(axis_labels) + self.assertTrue(all("_{" not in text for text in axis_labels)) + plt.close("all") + @pytest.mark.skipif( plt.get_backend().lower() == "agg" or os.environ.get("CI") == "true" diff --git a/tests/test_pose2d.py b/tests/test_pose2d.py index fc3734d6..4e4f7151 100755 --- a/tests/test_pose2d.py +++ b/tests/test_pose2d.py @@ -509,7 +509,7 @@ def test_graphics(self): T1.plot(block=False, dims=[-2, 2]) T1.animate(repeat=False, dims=[-2, 2], nframes=10) - T1.animate(T0=T2, repeat=False, dims=[-2, 2], nframes=10) + T1.animate(start=T2, repeat=False, dims=[-2, 2], nframes=10) # ---------------------------------------------------------------------------------------#