-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Add type annotations to cairo_renderer.py
#4393
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
@@ -186,7 +186,7 @@ def __init__( | |||
self.moving_mobjects: list[Mobject] = [] | |||
self.static_mobjects: list[Mobject] = [] | |||
self.time_progression: tqdm[float] | None = None | |||
self.duration: float | None = None | |||
self.duration: float = 0.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.
As far as I can tell setting self.duration = 0.0
, will not cause any problems, but it avoids having to deal with it being None
when calculating number of static frames or self.time
.
@@ -226,7 +233,7 @@ def save_static_frame_data( | |||
|
|||
Returns | |||
------- | |||
typing.Iterable[Mobject] | |||
Iterable[Mobject] | |||
The static image computed. | |||
""" | |||
self.static_image = 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.
For consistency with other methods and variables self.static_image
should ideally be renamed to self.static_frame
, but that can be done in another PR.
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 type annotations, especially those for manim.utils.hashing
! That's a pretty difficult module to type properly.
I left some change requests:
@@ -233,22 +234,22 @@ def default(self, obj: Any): | |||
# Serialize it with only the type of the object. You can change this to whatever string when debugging the serialization process. | |||
return str(type(obj)) | |||
|
|||
def _cleaned_iterable(self, iterable: Iterable[Any]): | |||
def _cleaned_iterable(self, iterable: Iterable[Any]) -> list[Any] | dict[Any, Any]: |
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.
Despite the name, iterable
can be either a Sequence
or a dict
, according to the code. Currently, iterable
can't be really an Iterable
, because _iter_check_list
calculates its length, something which not all iterables have (like map, range or filter).
EDIT: it is actually pretty easy to rewrite _iter_check_list
to accept any iterables. See my suggestion below.
The name of this parameter (and function) should probably be changed.
Now, we could simply type iterable
as Sequence[Any] | dict[Any, Any]
, but may I suggest the following overloads to indicate that, if iterable
is a Sequence
, the function returns a list
and, if it's a dict
, it returns a dict
:
def _cleaned_iterable(self, iterable: Iterable[Any]) -> list[Any] | dict[Any, Any]: | |
@overload | |
def _cleaned_iterable(self, iterable: Sequence[Any]) -> list[Any]: ... | |
@overload | |
def _cleaned_iterable(self, iterable: dict[Any, Any]) -> dict[Any, Any]: ... | |
def _cleaned_iterable(self, iterable): |
# mypy requires this line, even though it should not be reached. | ||
return iterable |
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.
MyPy requires this line, because, in the hypothetical case that _cleaned_iterable()
receives a different object that's neither an iterable or a dictionary, the function would implicitly return None
.
Now, as you mention, this shouldn't be reached, because the object should always be an iterable or a dictionary. In this case, I suggest raising an exception instead of silently returning the same iterable. In this way, if _cleaned_iterable()
is being passed something different, we can catch the bug instead of silently omitting it:
# mypy requires this line, even though it should not be reached. | |
return iterable | |
raise TypeError("'iterable' is neither an iterable nor a dictionary.") |
|
||
Parameters | ||
---------- | ||
iterable | ||
The iterable to check. | ||
""" | ||
|
||
def _key_to_hash(key): | ||
def _key_to_hash(key) -> int: |
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 _key_to_hash(key) -> int: | |
def _key_to_hash(key: Any) -> int: |
return zlib.crc32(json.dumps(key, cls=_CustomEncoder).encode()) | ||
|
||
def _iter_check_list(lst): | ||
def _iter_check_list(lst: list[Any]) -> list[Any]: |
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 _iter_check_list(lst: list[Any]) -> list[Any]: | |
def _iter_check_list(lst: Sequence[Any]) -> list[Any]: |
It's also technically pretty easy to rewrite this function to allow any iterables, but I'm not so sure about allowing potentially infinite iterables:
def _iter_check_list(lst: Iterable[Any]) -> list[Any]:
processed_list = []
for el in lst:
el = _Memoizer.check_already_processed(el)
if isinstance(el, Iterable):
new_value = _iter_check_list(el)
elif isinstance(el, dict):
new_value = _iter_check_dict(el)
else:
new_value = el
processed_list.append(new_value)
return processed_list
@@ -261,7 +262,7 @@ def _iter_check_list(lst): | |||
processed_list[i] = new_value | |||
return processed_list | |||
|
|||
def _iter_check_dict(dct): | |||
def _iter_check_dict(dct: dict) -> dict: |
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 _iter_check_dict(dct: dict) -> dict: | |
def _iter_check_dict(dct: dict[Any, Any]) -> dict[Any, Any]: |
@@ -285,8 +286,11 @@ def _iter_check_dict(dct): | |||
return _iter_check_list(iterable) |
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.
Can you please replace
if isinstance(iterable, (list, tuple)):
with
if isinstance(iterable, Sequence):
?
Lists and tuples pass that check.
@@ -324,7 +328,7 @@ def get_json(obj: dict): | |||
def get_hash_from_play_call( | |||
scene_object: Scene, | |||
camera_object: Camera | OpenGLCamera, | |||
animations_list: Iterable[Animation], | |||
animations_list: Iterable[Animation] | 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.
An issue with this is that animations_list
is expected to be an iterable inside the function. If it's None
, it will crash.
I noticed that CairoRenderer.play(self, scene, *args, **kwargs)
passes scene.animations
which is typed as list[Animation] | None
. However, since scene.compile_animation_data(*args, **kwargs)
is called before that, scene.animations
will always be a list[Animation]
at that point.
Therefore, instead of typing animations_list
as Iterable[Animation] | None
, you have to make an assertion inside CairoRenderer.play()
that scene.animations is not None
before the call to get_hash_from_play_call()
.
Overview: What does this pull request change?
More work towards completing #3375.
Motivation and Explanation: Why and how do your changes improve the library?
This PR includes some type annotations to
utils/hashing.py
to complete typing forcairo_renderer.py
. Typing forutils/hashing.py
should be completed in another PR.Reviewer Checklist