Skip to content

Conversation

@fmuenkel
Copy link
Contributor

Overview: What does this pull request change?

More work on #3375

Motivation and Explanation: Why and how do your changes improve the library?

Links to added or changed documentation pages

Further Information and Comments

Reviewer Checklist

  • The PR title is descriptive enough for the changelog, and the PR is labeled correctly
  • If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
  • If applicable: newly added functions and classes are tested

Copy link
Contributor

@chopan050 chopan050 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I request some small changes:

path_arc: float = 0,
path_arc_axis: np.ndarray = OUT,
path_arc_centers: np.ndarray = None,
path_arc_centers: Point3DLike = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to also include the possibility of None in the typehint, given that that's the default value.

Taking a look at path_along_circles(): back in the day, I typed the circles_centers parameter as Point3DLike_Array, based on the name (plural) and how the parameter was used inside that function. Now, upon closer inspection and noticing how Rotation.__init__() and test_TransformWithPathArcCenters() decide to pass a single point rather than an array of points, I realize that circles_centers can actually be Point3DLike | Point3DLike_Array. Thus, the type of path_arc_centers should actually be:

Suggested change
path_arc_centers: Point3DLike = None,
path_arc_centers: Point3DLike | Point3DLike_Array | None = None,

Feel free to also modify the typehints for path_along_circles() if you want to!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did the changes here, but I will have a look at the rest of transform.py as well in a separate PR.

Copy link
Contributor

@chopan050 chopan050 left a comment

Choose a reason for hiding this comment

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

LGTM!

@chopan050 chopan050 merged commit d938533 into ManimCommunity:main Jan 19, 2026
15 checks passed
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