-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add type annotations to rotation.py
#4535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
chopan050
left a comment
There was a problem hiding this 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:
manim/animation/transform.py
Outdated
| path_arc: float = 0, | ||
| path_arc_axis: np.ndarray = OUT, | ||
| path_arc_centers: np.ndarray = None, | ||
| path_arc_centers: Point3DLike = None, |
There was a problem hiding this comment.
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:
| 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!
There was a problem hiding this comment.
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.
chopan050
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
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