Add type annotations to mobject.py#4388
Add type annotations to mobject.py#4388henrikmidtiby wants to merge 48 commits intoManimCommunity:mainfrom
mobject.py#4388Conversation
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.
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.
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.
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.
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.
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]: |
manim/utils/color/core.py
Outdated
| """ | ||
| if length_of_output == 0: | ||
| return ManimColor(reference_colors[0]) | ||
| return [ManimColor(reference_colors[0])] |
| animation_overrides: dict[ | ||
| type[Animation], | ||
| FunctionOverride, | ||
| ] = {} |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
That's strange, but OK...
…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