Added support for iterables in VGroup constructor

[oscjac]

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

• The PR title is descriptive enough for the changelog, and the PR is labeled correctly
• If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
• If applicable: newly added functions and classes are tested

[JasonGrace2282]

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 ✨

[JasonGrace2282]

I would prefer this be under if TYPE_CHECKING since generators are counted as iterables

[JasonGrace2282]

Suggested change

	temp = []
	temp.extend(m)
	temp = tuple(m)

[JasonGrace2282]

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.

[JasonGrace2282]

Suggested change

	f"Got {repr(m)} ({type(m).__name__}) instead. "
	f"Got {repr(t)} ({type(t).__name__}) instead. "

Copy-paste error?

[JasonGrace2282]

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.

[JasonGrace2282]

Why not something like this?

Suggested change

	i = 0
	while i < n:
	i += 1
	yield type()
	for _ in range(n):
	yield type()

[JasonGrace2282]

Suggested change

	VGroup(basic_generator(Mobject))
	VGroup(basic_generator(1, Mobject))

Is this what you meant?

[JasonGrace2282]

See feedback above

[JasonGrace2282]

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)

[oscjac]

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 submobs 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?

[JasonGrace2282]

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 :)

[JasonGrace2282]

Closing due to inactivity - please reopen if this is not the case!