-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add type annotations to mobject.py
#4388
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
base: main
Are you sure you want to change the base?
Conversation
manim/mobject/geometry/line.py
Outdated
| # error: Argument "tip" to "add_tip" of "TipableVMobject" has incompatible type "VMobject"; expected "ArrowTip | None" [arg-type] | ||
| self.add_tip(tip=old_tips[0]) | ||
| if has_start_tip: | ||
| # error: Argument "tip" to "add_tip" of "TipableVMobject" has incompatible type "VMobject"; expected "ArrowTip | None" [arg-type] | ||
| self.add_tip(tip=old_tips[1], at_start=True) |
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.
These issues can be handled by typing.cast, but I don't think that is the best way to deal with them.
| def __getitem__(self, key: int) -> VMobject: | ||
| return self.submobjects[key] | ||
|
|
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.
This method is used to define the return type when indexing into a VGroup. If the super class was used, this would return a Mobject, but in a VGroup it should be a VMobject.
manim/scene/vector_space_scene.py
Outdated
| temp = self.get_basis_vectors() | ||
| i_hat = temp.submobjects[0] | ||
| j_hat = temp.submobjects[1] |
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.
Apparently the trick for defining the type of elements in a VGroup to be VMobject's does not work here. Which is the reason for this workaround.
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.
That's strange, but OK...
| temp = array.get_entries() | ||
| x_coord = temp.submobjects[0] | ||
| y_coord = temp.submobjects[1] |
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.
Apparently the trick for defining the type of elements in a VGroup to be VMobject's does not work here. Which is the reason for this workaround.
| reference_colors: Sequence[ParsableManimColor], | ||
| length_of_output: int, | ||
| ) -> list[ManimColor] | ManimColor: | ||
| ) -> list[ManimColor]: |
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.
Same change as in PR #4380
manim/utils/color/core.py
Outdated
| """ | ||
| if length_of_output == 0: | ||
| return ManimColor(reference_colors[0]) | ||
| return [ManimColor(reference_colors[0])] |
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.
Same change as in PR #4380.
| animation_overrides: dict[ | ||
| type[Animation], | ||
| FunctionOverride, | ||
| ] = {} |
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.
By specifying the type of animation_overrides here, a lot of type errors like "type[Mobject]" has no attribute "animation_overrides" are removed.
|
One of the main contributions in this PR is that when accessing elements in a This is achieved by adding/modifying these methods including type annotations to the And these to the |
… code for the remaning 10 errors
…in elements during runtime and not only during type checking
for more information, see https://pre-commit.ci
15d4832 to
f14e5c3
Compare
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 the huge contribution!
I left some suggestions:
| line[start:end].set_color(color) | ||
| if end == start: | ||
| continue | ||
| for single_line in line[start:end]: | ||
| single_line.set_color(color) |
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.
Checking both the original and the new code with MyPy locally, it shows no difference (in both cases, there is an error because Mobject.set_color() does not accept None as an argument), so I don't really understand the reason for this change...
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.
Strange, on my computer I get the following error with the original code when I run the pytest testsuite.
for line, color_range in zip(self.code_lines, color_ranges):
for start, end, color in color_range:
> line[start:end].set_color(color)
E AttributeError: 'list' object has no attribute 'set_color'
manim/mobject/mobject.py
Outdated
| # error: "Callable[..., Animation]" has no attribute "_override_animate" [attr-defined] | ||
| temp_method._override_animate = animation_method |
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.
Typing method so that it has a dynamic attribute _override_animate is difficult.
I propose the following alternative, maybe for another PR: create an _AnimationBuilder class dictionary _AnimationBuilder.animate_override_map: dict[int, types.MethodType] which maps method IDs to their .animate overrides. Thus, this line could be replaced with
_AnimationBuilder.animate_override_map[id(method)] = animation_methodInside _AnimationBuilder.__getattr__(), instead of doing
has_overriden_animation = hasattr(method, "_override_animate")we could do
method_override = self.animate_override_map.get(id(method), None)and then use it inside the internal function update_target().
In the meantime, let's just type ignore this one:
| # error: "Callable[..., Animation]" has no attribute "_override_animate" [attr-defined] | |
| temp_method._override_animate = animation_method | |
| method._override_animate = animation_method # type: ignore[attr-defined] |
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.
Lets take this in a separate PR.
manim/scene/vector_space_scene.py
Outdated
| temp = self.get_basis_vectors() | ||
| i_hat = temp.submobjects[0] | ||
| j_hat = temp.submobjects[1] |
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.
That's strange, but OK...
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 the huge contribution!
I left some suggestions. In particular, a raise ValueError(...) inside Mobject.arrange_in_grid() is causing an error while building the docs, so that one requires more attention.
…future cleanup task
Co-authored-by: Francisco Manríquez Novoa <49853152+chopan050@users.noreply.github.com>
for more information, see https://pre-commit.ci
| margin: float, | ||
| only_mobjects_in_frame: bool, | ||
| animate: Literal[False], | ||
| ) -> Mobject: ... |
Check notice
Code scanning / CodeQL
Statement has no effect
| margin: float, | ||
| only_mobjects_in_frame: bool, | ||
| animate: Literal[True], | ||
| ) -> _AnimationBuilder: ... |
Check notice
Code scanning / CodeQL
Statement has no effect
|
@chopan050 Thanks for all the comments, even the minor nitpicks! I feel they help me improving the PR. |
|
The I have attempted to solve the issue through implementing a |
# Conflicts: # manim/mobject/matrix.py # manim/mobject/mobject.py # manim/mobject/types/vectorized_mobject.py
Overview: What does this pull request change?
This PR add type annotations to
mobject.py.Issue #3375.
Reviewer Checklist