fix: support scalar/2-elem dim shorthand in tranimate/Animate - #202
Open
petercorke wants to merge 1 commit into
Open
fix: support scalar/2-elem dim shorthand in tranimate/Animate#202petercorke wants to merge 1 commit into
petercorke wants to merge 1 commit into
Conversation
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 Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tranimate(T, dim=1.5)— using the scalar-shorthanddimconventionestablished by
plotvol2/plotvol3elsewhere in this codebase — currentlycrashes with:
Two compounding bugs, both fixed here:
tranimate()popped"dims"(plural) out ofkwargsbefore forwardingto
Animate(dim=..., **kwargs), but every other function in thiscodebase (
plotvol2,plotvol3,Animate.__init__itself) names theparameter
dim(singular). A caller correctly usingdim=per thatconvention never got popped, so it stayed in
**kwargsand collidedwith the explicit
dim=dimalready being passed — hence "multiplevalues for keyword argument 'dim'".
Animate.__init__had its own hand-rolledlength check that only accepted a 2- or 6-element
dim, silentlydropping the scalar-shorthand convention (
A→[-A, A]on everyaxis) that
plotvol3already supports via the sharedexpand_dims()helper —
Animatejust never called it.Fix
tranimate()now acceptsdim, withdimskept as a back-compat aliasfor existing callers following the old docstring examples.
Animate.__init__now callsexpand_dims(dim, nd=3)(the same helperplotvol3uses) instead of its own incomplete validation, so scalar and2-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
dimsrename and thenew strict length check while trying to "expose
axanddimsargumentsto animate(...), to be consistent with other APIs." Before that commit,
tranimatepassed**kwargsstraight through toAnimate, and scalardimworked correctly viaAnimate's existingplotvol3delegation.Test plan
Animate(dim=1.5),Animate(dim=[0,5]),tranimate(R, dim=1.5, movie=True), andtranimate(R, dims=[0,5], movie=True)(back-compat alias) all construct/run correctly