Skip to content

fix(graphics): accept unspecified dimensions in expand_dims - #270

Open
Doribelove wants to merge 1 commit into
rai-opensource:masterfrom
Doribelove:fix/expand-dims-unspecified
Open

Doribelove wants to merge 1 commit into
rai-opensource:masterfrom
Doribelove:fix/expand-dims-unspecified

Conversation

@Doribelove

Copy link
Copy Markdown
Contributor

Fixes #261.

expand_dims() currently raises TypeError even though dim=None is its documented default. Return None unchanged for an unspecified extent so callers can decide whether to autoscale or obtain limits elsewhere. Update the argument and return annotations and docstring to describe this behavior.

Validate nd before returning, so unsupported dimensions still raise ValueError, including when dim is None. Add regressions for omitted and explicit None in 2D/3D, explicit zero and other numeric extents, malformed vectors, and unsupported nd. Numeric expansion remains unchanged; the scalar-sign behavior in #262 is outside this change.

Validation (MPLBACKEND=Agg):

  • The new default-value regression fails with TypeError before the fix.
  • With current upstream 41e0a4d: full suite 413 passed, 3 skipped, with 63 subtests passed and 22 warnings (Python 3.12.14, NumPy 2.5.3, Matplotlib 3.11.2).
  • The submitted branch is based on fork master f6a572c; its full suite is 349 passed, 3 skipped, with 8 subtests passed and 20 warnings. A local test merge into current upstream is conflict-free and produces exactly the tree used for the 413-test validation above.
  • Python 3.10.12: graphics tests 16 passed, with 8 subtests passed and 2 warnings.
  • Repository pre-commit hooks (Black 23.10.0, EOF, debug statements, trailing whitespace) and git diff --check pass. Black also wraps one existing long condition in the touched graphics module.

The existing internal callers already guard None. Downstream bdsim behavior has not been tested; the scale=None timing change noted in #261 remains applicable.

Signed-off-by: 李永祺 <doribelove@gmail.com>
@petercorke

Copy link
Copy Markdown
Collaborator

Thanks Dori, and thanks for the clear write-up. This is a nice tidy fix for #261.

I merged it with current master locally and the full suite passes (413 passed, 3 skipped). I also checked the four internal callers (plotvol2, plotvol3, Animate, and the dims handling in transforms2d.py). Each one tests for None before calling, so nothing changes for them, and I'm happy to leave them as they are.

Checking nd before the None return is a good catch, as is the 0 test showing it's treated as an explicit extent. Approving.

(Optional, not blocking: the Black re-wrap in _axes_dimensions is unrelated to this fix. It's fine to keep.)

@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

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

This branch has not been deployed

No deployments
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.

expand_dims(None) raises TypeError although None is its documented default

3 participants