Skip to content

fix: support scalar/2-elem dim shorthand in tranimate/Animate - #202

Open
petercorke wants to merge 1 commit into
rai-opensource:masterfrom
petercorke:fix/tranimate-scalar-dim
Open

fix: support scalar/2-elem dim shorthand in tranimate/Animate#202
petercorke wants to merge 1 commit into
rai-opensource:masterfrom
petercorke:fix/tranimate-scalar-dim

Conversation

@petercorke

Copy link
Copy Markdown
Collaborator

Summary

tranimate(T, dim=1.5) — using the scalar-shorthand dim convention
established by plotvol2/plotvol3 elsewhere in this codebase — currently
crashes with:

TypeError: spatialmath.base.animate.Animate() got multiple values for keyword argument 'dim'

Two compounding bugs, both fixed here:

  1. tranimate() popped "dims" (plural) out of kwargs before forwarding
    to Animate(dim=..., **kwargs), but every other function in this
    codebase (plotvol2, plotvol3, Animate.__init__ itself) names the
    parameter dim (singular). A caller correctly using dim= per that
    convention never got popped, so it stayed in **kwargs and collided
    with the explicit dim=dim already being passed — hence "multiple
    values for keyword argument 'dim'".
  2. Even with the right keyword, Animate.__init__ had its own hand-rolled
    length check that only accepted a 2- or 6-element dim, silently
    dropping the scalar-shorthand convention (A[-A, A] on every
    axis) that plotvol3 already supports via the shared expand_dims()
    helper — Animate just never called it.

Fix

  • tranimate() now accepts dim, with dims kept as a back-compat alias
    for existing callers following the old docstring examples.
  • Animate.__init__ now calls expand_dims(dim, nd=3) (the same helper
    plotvol3 uses) instead of its own incomplete validation, so scalar and
    2-element shorthand work consistently everywhere.

Regression context

This isn't a long-standing issue — it's a regression from #131 (Jien Cao,
2024-07-29, fixing #126), which introduced both the dims rename and the
new strict length check while trying to "expose ax and dims arguments
to animate(...), to be consistent with other APIs." Before that commit,
tranimate passed **kwargs straight through to Animate, and scalar
dim worked correctly via Animate's existing plotvol3 delegation.

Test plan

  • Verified directly: Animate(dim=1.5), Animate(dim=[0,5]),
    tranimate(R, dim=1.5, movie=True), and tranimate(R, dims=[0,5], movie=True) (back-compat alias) all construct/run correctly
  • Full existing test suite: 342 passed, no regressions

tranimate() popped "dims" from kwargs but forwarded it to Animate() as
dim=, and every other function in this codebase (plotvol2, plotvol3)
uses the singular "dim" as the actual keyword name. Calling
tranimate(T, dim=1.5) -- correct per that convention -- left dim=1.5
sitting in **kwargs, colliding with the explicit dim=dim already being
passed to Animate() and raising "got multiple values for keyword
argument 'dim'". Now accepts dim, with dims kept as a back-compat
alias for the previous docstring examples.

Animate.__init__ also had its own hand-rolled dim-length check that
only accepted 2 or 6 elements, so even a correctly-named dim=1.5 would
have failed validation afterward. Replaced it with expand_dims(), the
same helper plotvol3 already uses, so the scalar-shorthand convention
(A -> [-A,A] on every axis) works consistently everywhere.
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
spatialmath/base/animate.py 0.00% 1 Missing ⚠️
spatialmath/base/transforms3d.py 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants