-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Added support for iterables in VGroup constructor #3686
base: main
Are you sure you want to change the base?
Conversation
@@ -32,6 +33,7 @@ | |||
from manim.constants import * | |||
from manim.mobject.mobject import Mobject | |||
from manim.mobject.opengl.opengl_compatibility import ConvertToOpenGL | |||
from manim.mobject.opengl.opengl_mobject import OpenGLMobject |
Check notice
Code scanning / CodeQL
Cyclic import Note
manim.mobject.opengl.opengl_mobject
db7b4a6
to
19a6a38
Compare
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.
Hey, sorry for the late review. I've left some comments regarding what I noticed, feel free to take a look when you find the time!
Thanks for making manim better ✨
@@ -14,6 +14,7 @@ | |||
|
|||
import itertools as it | |||
import sys | |||
from types import GeneratorType |
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.
I would prefer this be under if TYPE_CHECKING
since generators are counted as iterables
temp = [] | ||
temp.extend(m) |
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.
temp = [] | |
temp.extend(m) | |
temp = tuple(m) |
# Mobject and its subclasses are iterable | ||
if isinstance(m, (Iterable, GeneratorType)): | ||
# If it's not a subclass of Mobject or OpenGLMobject, it must be an iterable or generator | ||
if not isinstance(m, (Mobject, OpenGLMobject)): |
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.
You could rewrite this in a way that reduces indentation by just checking if it's a Mobject
before checking if it's an iterable.
if not isinstance(t, (VMobject, OpenGLVMobject)): | ||
raise TypeError( | ||
f"All submobjects of {self.__class__.__name__} must be of type VMobject. " | ||
f"Got {repr(m)} ({type(m).__name__}) instead. " |
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.
f"Got {repr(m)} ({type(m).__name__}) instead. " | |
f"Got {repr(t)} ({type(t).__name__}) instead. " |
Copy-paste error?
def test_vgroup_iter_init(): | ||
"""Test the VGroup instantiation with an iterable type.""" | ||
|
||
def basic_generator(n, type): |
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.
I would suggest using a different parameter name other than type
(since it's a built-in function/metaclass/whatever you want to call it).
If you really think type
is the best name, use the name type_
as that's the naming standard for python.
i = 0 | ||
while i < n: | ||
i += 1 | ||
yield type() |
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.
Why not something like this?
i = 0 | |
while i < n: | |
i += 1 | |
yield type() | |
for _ in range(n): | |
yield type() |
obj = VGroup(basic_generator(5, VMobject)) | ||
assert len(obj.submobjects) == 5 | ||
with pytest.raises(TypeError): | ||
VGroup(basic_generator(Mobject)) |
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.
VGroup(basic_generator(Mobject)) | |
VGroup(basic_generator(1, Mobject)) |
Is this what you meant?
@@ -43,6 +43,22 @@ def test_vgroup_init(using_opengl_renderer): | |||
VGroup(OpenGLMobject(), OpenGLMobject()) | |||
|
|||
|
|||
def test_vgroup_iter_init(using_opengl_renderer): |
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.
See feedback above
for m in vmobjects: | ||
if not isinstance(m, (VMobject, OpenGLVMobject)): | ||
# Mobject and its subclasses are iterable | ||
if not isinstance(m, (Iterable, Generator)): |
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.
Perhaps I may not have been clear enough in my last review, but you could reduce the amount of explanation needed and the clarity of the code by doing the following:
if isinstance(mob, VMobject): # first
...
elif isinstance(mob, Iterable): # generator not needed, it's a subprotocol of Iterable
for submob in mob:
if not isinstance(submob, VMobject):
raise
flattened.extend(mob)
else:
raise
The benefit of this is that it's easier to read and it avoids repeating the same text for the ValueError
in 3 places (it repeats it twice, in the ideal case it should never repeat it)
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.
I see, thank you for the reviews! One problem that I'm running into testing this change locally is that when the constructor is passed an Mobject
instance, it does not throw a TypeError
.
The test is located in (tests/ ... /test_vectorized_mobject.py:77)
.
These are the changes I'm testing:
if isinstance(mob, VMobject):
flattened_args.append(mob)
elif isinstance(mob, Iterable):
for submob in mob:
if not isinstance(submob, VMobject):
raise TypeError(...)
flattened_args.extend(mob)
else:
raise TypeError(...)
The problem is that an empty Mobject
is iterable but, since it is empty, it has no submob
s and so the constructor never throws a TypeError
. My first thought was to add another check before iterating through the submob
as such:
elif isinstance(mob, Iterable):
if (mob, is instance(Mobject)):
raise TypeError(...)
for submob in mob:
if not isinstance(submob, VMobject):
raise TypeError(...)
flattened_args.extend(mob)
but, going off what you've already mentioned, I take it that it's not the best solution because it adds yet another copy of the TypeError and it is less readable. How do you think we could solve this without degrading the code's readability?
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.
Nice catch, didn't think about that. It could be circumvented with something like this
if isinstance(mob, VMobject):
...
elif isinstance(mob, Iterable) and type(mob) is not Mobject:
for submob in mob:
if not isinstance(submob, VMobject):
raise
flattened.extend(mob)
else:
raise
Not as clean as I like, but it should keep only two copies of the error message.
I would also suggest putting the error message into a function and simply call that function both times to reduce the repetition of the error.
Thanks for your patience :)
Overview: What does this pull request change?
Also includes tests to verify that the change works as intended. This change does not appear to affect other existing tests, other than adding a negligible amount of runtime cost to calling the Vgroup constructor. Closes #3540
Reviewer Checklist