Skip to content
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

Allow VGroup type subscripting #3606

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

JasonGrace2282
Copy link
Member

@JasonGrace2282 JasonGrace2282 commented Feb 1, 2024

Allows things like
VGroup[Rectangle]

[v1]

Adds a __class_getitem__ method to Mobject

This was not useful, as discussed later

[v2]

Make VGroup a generic

Guideline

Since VGroup's are invariant, type VGroup's similar to lists. For example:

Correct:

def get_lines() -> VGroup[Line]:
    return VGroup(Line(), Line()).arrange()

Incorrect:

def get_lines() -> VGroup[Line, Line]:
    return VGroup(Line(), Line()).arrange()

Also renders correctly in the docs, ex LinearTransformationScene.get_ghost_vectors

@JasonGrace2282 JasonGrace2282 added the enhancement Additions and improvements in general label Feb 1, 2024
@JasonGrace2282 JasonGrace2282 marked this pull request as draft February 1, 2024 21:42
@JasonGrace2282

This comment has been minimized.

@JasonGrace2282 JasonGrace2282 marked this pull request as ready for review February 4, 2024 22:55
@JasonGrace2282 JasonGrace2282 added this to the v0.18.1 milestone Feb 13, 2024
@Viicos
Copy link
Member

Viicos commented Feb 13, 2024

What are you trying to achieve with this feature? VGroup[Rectangle] seems to be a good use case for generics, but in that case lets inherit from Generic and define the correct type variable(s), as currently the __class_getitem__ has no value for type checkers.

@JasonGrace2282
Copy link
Member Author

JasonGrace2282 commented Feb 13, 2024

lets inherit from Generic and define the correct type variable(s)

I agree that would be the best solution, but I wanted to avoid (if possible) having to manually make every class a ParentClass[VMobject] or whatever. If there's no other way, I can do that.
Please correct me if I have any misunderstandings, as I've never used Generic classes before.

currently the __class_getitem__ has no value for type checkers.

Oh I didn't know that, thanks for letting me know :)

@Viicos
Copy link
Member

Viicos commented Feb 13, 2024

Having VGroup a generic class can make sense. It would look like:

class VGroup(VMobject, Generic[T], metaclass=ConvertToOpenGL):
    def __init__(self, *vmobjects: T, **kwargs):
        super().__init__(**kwargs)
        self.add(*vmobjects)

But it would imply VGroups can only hold one vmobject type, is it the case?


Mobject[Mobject] doesn't really make sense, the general (but not limited to) use case for generics is to type check values of a container (such as VGroup).


I agree that would be the best solution, but I wanted to avoid (if possible) having to manually make every class a ParentClass[VMobject] or whatever.

Not sure I understood this part, could you please give an example?

@JasonGrace2282
Copy link
Member Author

JasonGrace2282 commented Feb 13, 2024

But it would imply VGroups can only hold one vmobject type, is it the case?

It is not in fact the case, but maybe something like T | VMobject could fix it

Not sure I understood this part, could you please give an example?

Sure, ideally we would only change what VGroup inherits from, and every subclass "just works". However, if we inherit from Generic[T] every subclass of VGroup must then inherit from VGroup[T] instead of just VGroup.

class VGroup(VMobject, Generic[T], metaclass=ConvertToOpenGL):
    def __init__(self, *vmobjects: T, **kwargs):
        super().__init__(**kwargs)
        self.add(*vmobjects)
class Subclass1(VGroup):
    pass
class Subclass2(VGroup[T]):
    pass

Subclass1[VMobject] # error
Subclass2[VMobject] # no error

It was just me being lazy and not wanting to have to create a TypeVar("T") everywhere. If you think it's the better thing to do, I'll trust your judgement.

@Viicos
Copy link
Member

Viicos commented Feb 14, 2024

I see. While this might look cumbersome to add the type variable to every subclass, this is required for type checkers to understand the generic class. Adding a bare __class_getitem__ will provide no valuable info.

In my opinion, the type variable should only be added to VGroup. Mobject also has submobjects, but it isn't really common to deal with it for plain mobjects.

It is not in fact the case, but maybe something like T | VMobject could fix it

You can do something like:

VMobjectT = TypeVar("VMobjectT", bound=VMobject)

class VGroup(VMobject, Generic[VMobjectT], metaclass=ConvertToOpenGL):

    def __init__(self, *vmobjects: VMobjectT, **kwargs): ...

That way, users that use different vmobjects in a vgroup won't be impacted:

g = VGroup(DashedVMobject(), DashedVMobject())
reveal_type(g)  # VGroup[DashedVMobject()]

g = VGroup(DashedVMobject(), Triangle())
reveal_type(g)  # "VGroup[DashedVMobject | Triangle]" for pyright | "VGroup[VMobject]" for mypy

We could also make use of PEP 696 in this case. It is still in draft but will most probably make it:

from typing_extensions import TypeVar

VMobjectT = TypeVar("VMobjectT", bound=VMobject, default=VMobject)

That way a plain VGroup annotation will still have submobjects annotated as list[VMobject].


As you can see, it is not perfect as mypy and pyright handle unions differently. But that could still work

@JasonGrace2282
Copy link
Member Author

And I assume all subclasses would be subclassing VGroup[VMobject] (unless of course it's possible to be more specific)?
e.g.

class ManimBanner(VGroup[VMobject]):
    ...

@Viicos
Copy link
Member

Viicos commented Feb 16, 2024

And I assume all subclasses would be subclassing VGroup[VMobject] (unless of course it's possible to be more specific)?

If the subclass behaves the same way VGroup does, it can still be parametrized by the user, so ManimBanner(VGroup[VMobjectT]) would be correct

@JasonGrace2282 JasonGrace2282 marked this pull request as draft February 16, 2024 21:28
@JasonGrace2282 JasonGrace2282 changed the title Add :meth:.Mobject.__class_getitem__ to allow type subscripting Allow VGroup type subscripting Feb 16, 2024
@JasonGrace2282 JasonGrace2282 added breaking changes This PR introduces breaking changes and removed breaking changes This PR introduces breaking changes labels Feb 20, 2024
@JasonGrace2282 JasonGrace2282 marked this pull request as ready for review February 27, 2024 03:45
@JasonGrace2282 JasonGrace2282 requested a review from Viicos March 11, 2024 23:33
manim/mobject/text/text_mobject.py Fixed Show fixed Hide fixed
manim/mobject/vector_field.py Fixed Show resolved Hide resolved
@JasonGrace2282
Copy link
Member Author

Seeing as PEP 696 was accepted I've gone ahead and made the changes, let me know what you think!

@JasonGrace2282 JasonGrace2282 added the typehints For adding/discussing typehints label Apr 18, 2024
@behackl behackl modified the milestones: v0.18.1, v0.19.0 Apr 24, 2024
@JasonGrace2282 JasonGrace2282 force-pushed the VGroup_type_subscripting branch 2 times, most recently from 9caa588 to 99c3241 Compare April 30, 2024 11:42
@Viicos
Copy link
Member

Viicos commented May 7, 2024

@JasonGrace2282,

this week I'll have time to review your typing related PRs.

@JasonGrace2282 JasonGrace2282 force-pushed the VGroup_type_subscripting branch from d4a45d2 to 1d895b2 Compare May 12, 2024 02:57
@JasonGrace2282 JasonGrace2282 modified the milestones: v0.19.0, v0.20.0 Jul 23, 2024
)
from manim.mobject.geometry.line import Line
from manim.mobject.geometry.polygram import RoundedRectangle
from manim.mobject.mobject import Mobject
from manim.mobject.types.vectorized_mobject import VGroup
from manim.utils.color import BLACK, RED, YELLOW, ManimColor, ParsableManimColor

if TYPE_CHECKING:

Check failure

Code scanning / CodeQL

Module-level cyclic import Error

'VGroup' may not be defined if module
manim.mobject.types.vectorized_mobject
is imported before module
manim.mobject.geometry.shape_matchers
, as the
definition
of VGroup occurs after the cyclic
import
of manim.mobject.geometry.shape_matchers.
'VGroup' may not be defined if module
manim.mobject.types.vectorized_mobject
is imported before module
manim.mobject.geometry.shape_matchers
, as the
definition
of VGroup occurs after the cyclic
import
of manim.mobject.geometry.shape_matchers.
@@ -70,7 +70,7 @@
from manim.constants import *
from manim.mobject.geometry.arc import Dot
from manim.mobject.svg.svg_mobject import SVGMobject
from manim.mobject.types.vectorized_mobject import VGroup, VMobject
from manim.mobject.types.vectorized_mobject import VGroup, VMobject, VMobjectT

Check failure

Code scanning / CodeQL

Module-level cyclic import Error

'VGroup' may not be defined if module
manim.mobject.types.vectorized_mobject
is imported before module
manim.mobject.text.text_mobject
, as the
definition
of VGroup occurs after the cyclic
import
of manim.mobject.text.text_mobject.
'VGroup' may not be defined if module
manim.mobject.types.vectorized_mobject
is imported before module
manim.mobject.text.text_mobject
, as the
definition
of VGroup occurs after the cyclic
import
of manim.mobject.text.text_mobject.
@@ -70,7 +70,7 @@
from manim.constants import *
from manim.mobject.geometry.arc import Dot
from manim.mobject.svg.svg_mobject import SVGMobject
from manim.mobject.types.vectorized_mobject import VGroup, VMobject
from manim.mobject.types.vectorized_mobject import VGroup, VMobject, VMobjectT

Check failure

Code scanning / CodeQL

Module-level cyclic import Error

'VMobject' may not be defined if module
manim.mobject.types.vectorized_mobject
is imported before module
manim.mobject.text.text_mobject
, as the
definition
of VMobject occurs after the cyclic
import
of manim.mobject.text.text_mobject.
'VMobject' may not be defined if module
manim.mobject.types.vectorized_mobject
is imported before module
manim.mobject.text.text_mobject
, as the
definition
of VMobject occurs after the cyclic
import
of manim.mobject.text.text_mobject.
@@ -70,7 +70,7 @@
from manim.constants import *
from manim.mobject.geometry.arc import Dot
from manim.mobject.svg.svg_mobject import SVGMobject
from manim.mobject.types.vectorized_mobject import VGroup, VMobject
from manim.mobject.types.vectorized_mobject import VGroup, VMobject, VMobjectT

Check failure

Code scanning / CodeQL

Module-level cyclic import Error

'VMobjectT' may not be defined if module
manim.mobject.types.vectorized_mobject
is imported before module
manim.mobject.text.text_mobject
, as the
definition
of VMobjectT occurs after the cyclic
import
of manim.mobject.text.text_mobject.
'VMobjectT' may not be defined if module
manim.mobject.types.vectorized_mobject
is imported before module
manim.mobject.text.text_mobject
, as the
definition
of VMobjectT occurs after the cyclic
import
of manim.mobject.text.text_mobject.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Additions and improvements in general typehints For adding/discussing typehints
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

3 participants