Skip to content

fix(graphing): align coordinate conversion type contracts - #4951

Open
emerardd wants to merge 2 commits into
ManimCommunity:mainfrom
emerardd:fix/coordinate-system-type-contract
Open

fix(graphing): align coordinate conversion type contracts#4951
emerardd wants to merge 2 commits into
ManimCommunity:mainfrom
emerardd:fix/coordinate-system-type-contract

Conversation

@emerardd

Copy link
Copy Markdown
Contributor

Closes #4804.

Summary

  • Align the CoordinateSystem and Axes coordinate-conversion type contracts with their existing single-point and batch behavior.
  • Apply the same accepted input forms to coords_to_point, point_to_coords, their c2p / p2c aliases, and the forward/reverse @ operators.
  • Use np.ndarray for return values so the annotations cover both point batches shaped (M, 3) and component-wise batches shaped (3, M).
  • Rename converted local arrays to avoid reassigning parameters with a different static type; runtime behavior is unchanged.

Validation

  • ruff check --no-cache manim/mobject/graphing/coordinate_systems.py
  • Ruff formatting check
  • Targeted mypy caller-contract probe covering ordinary floats, sequences, NumPy arrays, batch inputs, aliases, both @ directions, and ThreeDAxes
  • 10 direct runtime contract assertions covering single-point and both batch layouts
  • git diff --check

@emerardd
emerardd marked this pull request as ready for review August 21, 2026 15:17

@behackl behackl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, and thanks for contributing! It is a worthwhile task to expand the existing type hints to reflect that sequences can be input too.

However, I'd prefer if you'd write them out using the existing type aliases from manim.typing instead of writing them as these multi-unions.

For example, point could be typed as Point3DLike | Point3DLike_Array, while coord could be PointNDLike | PointNDLike_Array | Mobject.

(And while I know that this is a preexisting problem because numpy arrays are described by Point3DLike too: actually trying to multiply <numpy array> @ axes errors out because numpy interprets the @ as a matrix multiplication that effectively fails.)

Please take a look and let me know whether you agree.

@emerardd

emerardd commented Aug 26, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review — I agree. The expanded unions duplicated the shape semantics already captured by the aliases in manim.typing and made the signatures harder to read.

Updated in a86a40f3:

  • scene-space point inputs now use Point3DLike | Point3DLike_Array;
  • coordinate inputs now use PointNDLike | PointNDLike_Array;
  • np.ndarray remains the return annotation because the existing methods support both row-wise and component-wise batch layouts;
  • the branch has been rebased onto the latest main.

I also agree that numpy.ndarray @ axes is a pre-existing NumPy dispatch limitation; this PR does not imply that left-hand NumPy matrix multiplication is newly supported, and the runtime behavior remains unchanged.

The focused coordinate tests, Ruff, targeted mypy checks, and runtime contract checks pass. Could you take another look when convenient?

@emerardd
emerardd force-pushed the fix/coordinate-system-type-contract branch from 12eb8c7 to a86a40f Compare August 26, 2026 08:51
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.

graphing: align coords_to_point return type in base class with subclass behaviour

2 participants