-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Adding type annotations to manim.utils.* #3999
base: main
Are you sure you want to change the base?
Changes from all commits
6e0d440
8c887b5
09b10c7
5868534
57d4971
f764767
1021c47
0002711
df742bc
ce85521
21c1a44
e155bae
2f5480d
97227b2
3a2eaee
d21525d
1b1e417
7bb63bd
a26da20
3b2e31d
b1e815e
f086fec
d51f929
b0bd14e
2f151d7
224db6d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -692,7 +692,7 @@ def digest_parser(self, parser: configparser.ConfigParser) -> Self: | |||||
|
||||||
return self | ||||||
|
||||||
def digest_args(self, args: argparse.Namespace) -> Self: | ||||||
def digest_args(self, args: argparse.Namespace | list[str]) -> Self: | ||||||
"""Process the config options present in CLI arguments. | ||||||
Parameters | ||||||
|
@@ -1395,7 +1395,7 @@ def renderer(self, value: str | RendererType) -> None: | |||||
self._set_from_enum("renderer", renderer, RendererType) | ||||||
|
||||||
@property | ||||||
def media_dir(self) -> str: | ||||||
def media_dir(self) -> str | Path: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line 29 in option(
"--media_dir",
type=Path(),
default=None,
help="Path to store rendered videos and latex.",
), which means it will be always parsed as a Technically, all those properties can return
Suggested change
|
||||||
"""Main output directory. See :meth:`ManimConfig.get_dir`.""" | ||||||
return self._d["media_dir"] | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -3,6 +3,7 @@ | |||||
import typing | ||||||
|
||||||
import numpy as np | ||||||
import numpy.typing as npt | ||||||
|
||||||
from manim.utils.hashing import get_hash_from_play_call | ||||||
|
||||||
|
@@ -160,7 +161,7 @@ def render(self, scene, time, moving_mobjects): | |||||
self.update_frame(scene, moving_mobjects) | ||||||
self.add_frame(self.get_frame()) | ||||||
|
||||||
def get_frame(self): | ||||||
def get_frame(self) -> npt.NDArray: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a type alias in
Suggested change
|
||||||
""" | ||||||
Gets the current frame as NumPy array. | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -47,7 +47,7 @@ | |||||
from ..utils import opengl, space_ops | ||||||
from ..utils.exceptions import EndSceneEarlyException, RerunSceneException | ||||||
from ..utils.family import extract_mobject_family_members | ||||||
from ..utils.family_ops import restructure_list_to_exclude_certain_family_members | ||||||
Check failure Code scanning / CodeQL Module-level cyclic import Error
'restructure_list_to_exclude_certain_family_members' may not be defined if module
manim.utils.family_ops Error loading related location Loading manim.scene.scene Error loading related location Loading definition Error loading related location Loading import Error loading related location Loading |
||||||
from ..utils.file_ops import open_media_file | ||||||
from ..utils.iterables import list_difference_update, list_update | ||||||
|
||||||
|
@@ -486,7 +486,7 @@ | |||||
self.moving_mobjects += mobjects | ||||||
return self | ||||||
|
||||||
def add_mobjects_from_animations(self, animations): | ||||||
def add_mobjects_from_animations(self, animations: list) -> None: | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When a parameter or return type is a
Suggested change
|
||||||
curr_mobjects = self.get_mobject_family_members() | ||||||
for animation in animations: | ||||||
if animation.is_introducer(): | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -20,7 +20,7 @@ | |||||
from pydub import AudioSegment | ||||||
|
||||||
from manim import __version__ | ||||||
from manim.typing import PixelArray | ||||||
from manim.typing import PixelArray, StrPath | ||||||
|
||||||
from .. import config, logger | ||||||
from .._config.logger_utils import set_file_logger | ||||||
|
@@ -34,7 +34,7 @@ | |||||
modify_atime, | ||||||
write_to_movie, | ||||||
) | ||||||
from ..utils.sounds import get_full_sound_file_path | ||||||
Check failure Code scanning / CodeQL Module-level cyclic import Error
'get_full_sound_file_path' may not be defined if module
manim.utils.sounds Error loading related location Loading manim.scene.scene_file_writer Error loading related location Loading definition Error loading related location Loading import Error loading related location Loading |
||||||
from .section import DefaultSectionType, Section | ||||||
|
||||||
if TYPE_CHECKING: | ||||||
|
@@ -104,7 +104,12 @@ | |||||
|
||||||
force_output_as_scene_name = False | ||||||
|
||||||
def __init__(self, renderer, scene_name, **kwargs): | ||||||
def __init__( | ||||||
self, | ||||||
renderer: Any, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
In this way, the type checker can detect the |
||||||
scene_name: StrPath, | ||||||
**kwargs: Any, | ||||||
) -> None: | ||||||
self.renderer = renderer | ||||||
self.init_output_directories(scene_name) | ||||||
self.init_audio() | ||||||
|
@@ -118,7 +123,7 @@ | |||||
name="autocreated", type_=DefaultSectionType.NORMAL, skip_animations=False | ||||||
) | ||||||
|
||||||
def init_output_directories(self, scene_name): | ||||||
def init_output_directories(self, scene_name: StrPath): | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
"""Initialise output directories. | ||||||
|
||||||
Notes | ||||||
|
@@ -378,7 +383,9 @@ | |||||
self.add_audio_segment(new_segment, time, **kwargs) | ||||||
|
||||||
# Writers | ||||||
def begin_animation(self, allow_write: bool = False, file_path=None): | ||||||
def begin_animation( | ||||||
self, allow_write: bool = False, file_path: StrPath | None = None | ||||||
) -> None: | ||||||
""" | ||||||
Used internally by manim to stream the animation to FFMPEG for | ||||||
displaying or writing to a file. | ||||||
|
@@ -391,7 +398,7 @@ | |||||
if write_to_movie() and allow_write: | ||||||
self.open_partial_movie_stream(file_path=file_path) | ||||||
|
||||||
def end_animation(self, allow_write: bool = False): | ||||||
def end_animation(self, allow_write: bool = False) -> None: | ||||||
""" | ||||||
Internally used by Manim to stop streaming to | ||||||
FFMPEG gracefully. | ||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -35,6 +35,8 @@ | |||||||||||||||||||||
BezierPoints, | ||||||||||||||||||||||
BezierPoints_Array, | ||||||||||||||||||||||
ColVector, | ||||||||||||||||||||||
InternalPoint3D, | ||||||||||||||||||||||
InternalPoint3D_Array, | ||||||||||||||||||||||
MatrixMN, | ||||||||||||||||||||||
Point3D, | ||||||||||||||||||||||
Point3D_Array, | ||||||||||||||||||||||
|
@@ -54,10 +56,12 @@ | |||||||||||||||||||||
@overload | ||||||||||||||||||||||
def bezier( | ||||||||||||||||||||||
points: Sequence[Point3D_Array], | ||||||||||||||||||||||
) -> Callable[[float | ColVector], Point3D_Array]: ... | ||||||||||||||||||||||
) -> Callable[[float | ColVector], Point3D | Point3D_Array]: ... | ||||||||||||||||||||||
|
) -> Callable[[float | ColVector], Point3D | Point3D_Array]: ... | |
) -> Callable[[float | ColVector], Point3D_Array]: ... |
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.
Please prefer the MatrixMN
type alias:
SUBDIVISION_MATRICES: list[dict[int, npt.NDArray]] = [{} for i in range(4)] | |
SUBDIVISION_MATRICES: list[dict[int, MatrixMN]] = [{} for i in range(4)] |
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.
At the time this PR was made (see explanation below), the previous parameter type was correct: points
is an array containing the control points for a Bézier curve, so BezierPoints
is more appropiate than (Internal)Point3D_Array
.
Actually, the previous return type was imprecise: it should've been Spline
instead of Point3D_Array
. Spline
is a more specific kind of Point3D_Array
consisting of consecutive blocks of control points for consecutive Bézier curves which also happen to be connected, forming a continuous curve.
There is a recently merged PR which renames type aliases such as InternalPoint3D
(a NumPy array) to Point3D
, and Point3D
(anything resembling a 3D point) to Point3DLike
: #4027
Therefore, since points
doesn't have to be a NumPy array (because it is converted to one inside the function), the correct type would now be BezierPointsLike
.
def subdivide_bezier( | |
points: InternalPoint3D_Array, n_divisions: int | |
) -> InternalPoint3D_Array: | |
def subdivide_bezier(points: BezierPointsLike, n_divisions: int) -> Spline: |
Dismissed
Show dismissed
Hide dismissed
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.
When this PR was made, it was correct to type start
and end
as InternalPoint3D
. Actually, the same should have been applied to the return type.
Now, since PR #4027, it's correct to type all of it as Point3D
which now always represents a NumPy array, in contrast with the new Point3DLike
which can also be a tuple or list of floats.
def interpolate( | |
start: InternalPoint3D, end: InternalPoint3D, alpha: float | |
) -> Point3D: ... | |
def interpolate(start: Point3D, end: Point3D, alpha: float) -> Point3D: ... |
Dismissed
Show dismissed
Hide dismissed
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 as before:
def interpolate( | |
start: InternalPoint3D, end: InternalPoint3D, alpha: ColVector | |
) -> Point3D_Array: ... | |
def interpolate(start: Point3D, end: Point3D, alpha: ColVector) -> Point3D_Array: ... |
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.
def interpolate( | |
start: float | InternalPoint3D, | |
end: float | InternalPoint3D, | |
alpha: float | ColVector, | |
) -> float | ColVector | Point3D | Point3D_Array: | |
def interpolate( | |
start: float | Point3D, | |
end: float | Point3D, | |
alpha: float | ColVector, | |
) -> float | ColVector | Point3D | Point3D_Array: |
Dismissed
Show dismissed
Hide dismissed
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.
def mid(start: InternalPoint3D, end: InternalPoint3D) -> Point3D: ... | |
def mid(start: Point3D, end: Point3D) -> Point3D: ... |
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.
def mid( | |
start: float | InternalPoint3D, end: float | InternalPoint3D | |
) -> float | Point3D: | |
def mid(start: float | Point3D, end: float | Point3D) -> float | Point3D: |
Check notice
Code scanning / CodeQL
Statement has no effect Note
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.
def inverse_interpolate( | |
start: float, end: float, value: InternalPoint3D | |
) -> InternalPoint3D: ... | |
def inverse_interpolate(start: float, end: float, value: Point3D) -> Point3D: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
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.
def inverse_interpolate( | |
start: InternalPoint3D, end: InternalPoint3D, value: InternalPoint3D | |
) -> Point3D: ... | |
def inverse_interpolate(start: Point3D, end: Point3D, value: Point3D) -> Point3D: ... |
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.
start: float | InternalPoint3D, | |
end: float | InternalPoint3D, | |
value: float | InternalPoint3D, | |
start: float | Point3D, | |
end: float | Point3D, | |
value: float | Point3D, |
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.
old_value: float | InternalPoint3D, | |
old_value: float | Point3D, |
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 and the return types of the previous overloads are not correct, because it's not the control points for a single Bézier curve, but rather 2 or 2N curves. It should be QuadraticSpline
for the first overload with Point3D
, because it's 2 connected quadratic Béziers, and QuadraticBezierPath
for the second overload with Point3D_Array
, because it's 2N quadratic Béziers which are not necessarily connected, depending on the given points.
) -> QuadraticBezierPoints_Array: | |
) -> QuadraticSpline | QuadraticBezierPath: |
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.
def is_closed(points: InternalPoint3D_Array) -> bool: | |
def is_closed(points: Point3D_Array) -> bool: |
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.
args
cannot be alist[str]
in this case.args
is an object with multiple attributes which a list doesn't have, as it can be seen in lines such as: