Draft: Add type annotations to mobject.py #2129

Timmmm · 2021-10-01T22:07:21Z

This is a draft - it's not quite finished yet; I just put it up for discussion (and in case I never finish it and somebody wants to take over). Or in case you just want to land it unfinished.

This adds quite a few type annotations to mobject.py (and a few to other files to help). Type checked with Pyright - MyPy basically can't handle this at all. There are quite a few type errors left but way fewer than when I started (Pyright gave up about half way through the file!)

Some misc notes:

The code is not clear where Sequence[float] is allowed and where np.ndarray is (or either). I've mostly just put np.ndarray.
Similarly for Color vs str. Not clear where either is allowed and some of the existing type annotations seem to be wrong.
Changed np.float64 to float for simplicity. They're officialy not compatible but they actually are the same.
Removed unused axes parameter from rotate_about_origin.

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

This adds quite a few type annotations to mobject.py (and a few to other files to help). Type checked with Pyright - MyPy basically can't handle this at all. There are quite a few type errors left but way fewer than when I started (Pyright gave up about half way through the file!) Some misc notes: * The code is not clear where `Sequence[float]` is allowed and where `np.ndarray` is (or either). I've mostly just put `np.ndarray`. * Similarly for `Color` vs `str`. Not clear where either is allowed and some of the existing type annotations seem to be wrong. * Changed `np.float64` to `float` for simplicity. They're officialy not compatible but they actually are the same. * Removed unused `axes` parameter from `rotate_about_origin`.

manim/mobject/mobject.py

Darylgolden · 2021-10-02T02:54:45Z

Just a comment about writing unions as X | Y - since this is only going to be introduced with Python 3.10, and we're officially supporting Python 3.7+, you should use the old syntax.

GameDungeon · 2021-10-02T03:18:01Z

This is similar to a pull I already made (but you did better), where I made a dedicated file for this. I think the changes should be combined: #2052

GameDungeon · 2021-10-02T03:19:18Z

It seems to really not like your _typeshed

GameDungeon · 2021-10-02T03:21:05Z

manim/mobject/mobject.py


 import numpy as np
+
+# TODO(types): colour has no type annotations.


Would be fixed by either #1972 or #2050

I don't think so. This is about this colour not the Manim Color type.

Though you have just made me realise that I put Color in loads of places where I think it should have been Colors. I'll fix it.

GameDungeon · 2021-10-02T03:22:03Z

manim/mobject/mobject.py

+    color: Color | ColorStr | None
+    name: str
+    dim: int
+    target: Mobject | None
+    z_index: float
+    point_hash: int | None


Like already mentioned, those are not supported yet. Use Union.

manim/mobject/mobject.py

GameDungeon · 2021-10-02T03:24:15Z

manim/mobject/mobject.py

@@ -499,13 +541,13 @@ def remove(self, *mobjects: "Mobject") -> "Mobject":
                self.submobjects.remove(mobject)
        return self

-    def __sub__(self, other):
+    def __sub__(self, other: Mobject) -> Mobject:


Lying about the return value

Do you mean the return type is wrong? If so what should it be? It's pretty hard to tell in some cases!

I don't think you mean lying - "lying" means being deliberately deceptive and I hope you aren't accusing me of that!

GameDungeon · 2021-10-02T03:24:20Z

manim/mobject/mobject.py

        raise NotImplementedError

-    def __isub__(self, other):
+    def __isub__(self, other: Mobject) -> Mobject:


Lying about the return value

This is similar to a pull I already made (but you did better), where I made a dedicated file for this. I think the changes should be combined: #2052

Nice, I had a look and I think our changes don't conflict much (if at all) so hopefully we can land them separately.

manim/mobject/mobject.py

GameDungeon · 2021-10-02T03:28:42Z

manim/mobject/mobject.py

+Updater = Union[Callable[["Mobject"], None], Callable[["Mobject", float], None]]
+# Any subclass of Mobject. Most Mobject methods `return self` so this is used
+# so that e.g. Circle.scale(2) returns a Circle, not an Mobject.
+MOS = TypeVar("MOS", bound="Mobject")


Maybe a better name for this? I mean it's not needed. Maybe MOB instead of MOS

"MOS" was meant to be "MObject Subclass"... Maybe that was not obvious - happy to change it! Why MOB? Just MOBject?

Timmmm · 2021-10-02T12:38:40Z

Just a comment about writing unions as X | Y - since this is only going to be introduced with Python 3.10, and we're officially supporting Python 3.7+, you should use the old syntax.

Actually I'm pretty sure from __future__ import annotations takes care of that. I don't have Python 3.7 to hand but it works fine in Python 3.8:

~ python3
Python 3.8.7 (default, Jan  9 2021, 01:55:12) 
[Clang 9.0.0 (tags/RELEASE_900/final)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> x: int | None = 5
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'
>>> from __future__ import annotations
>>> x: int | None = 5
>>> x
5

I think it doesn't work in some contexts, but Pyright seems to know the rules and give an error when you get it wrong. Do you have tests that run it with Python 3.7 just to be sure?

Timmmm · 2021-10-02T12:47:06Z

I think it doesn't work in some contexts

Yeah so the thing you can't do until Python 3.10 (even with from __future__ import annotations) is use | in a non type-hint context. In other words this is fine:

x: int | str = 5

But this isn't

IntOrStr = int | str

Pyright warns you an error for the second case.

…typeshed, remove accidental `Color`s

GameDungeon · 2021-10-02T13:48:09Z

I think it doesn't work in some contexts, but Pyright seems to know the rules and give an error when you get it wrong. Do you have tests that run it with Python 3.7 just to be sure?

I mean look at our CI, we do have tests

Timmmm · 2021-10-02T15:39:10Z

I mean look at our CI, we do have tests

Alright, I was just asking. No need to get defensive!

Darylgolden · 2022-01-14T01:16:55Z

Hi! Are you still interested in working on this?

Darylgolden · 2022-01-22T05:51:50Z

I'll close this for now, please feel free to reopen if you intend to continue working on this!

Timmmm · 2022-01-22T10:39:16Z

Hey, yeah sorry for not replying. I took this about as far as I could but to be honest didn't see a whole lot of interest and it really needs someone familiar with the code and with commit access to push it through. (It's often hard to divine types without knowing the intent of the code.)

I'm going to try https://github.com/imaphatduc/cubecubed for my next project. Terrible name! But it's written in Typescript so hopefully it's easier to follow, and it outputs to the web which is cool.

Timmmm mentioned this pull request Oct 1, 2021

Add typings #52

Closed

Timmmm commented Oct 1, 2021

View reviewed changes

manim/mobject/mobject.py Outdated Show resolved Hide resolved

behackl marked this pull request as draft October 1, 2021 23:59

GameDungeon suggested changes Oct 2, 2021

View reviewed changes

Add deprecations back (and improve @deprecated type hints), remove …

3b446f8

…typeshed, remove accidental `Color`s

Darylgolden closed this Jan 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft: Add type annotations to mobject.py #2129

Draft: Add type annotations to mobject.py #2129

Timmmm commented Oct 1, 2021

Darylgolden commented Oct 2, 2021

GameDungeon commented Oct 2, 2021

GameDungeon commented Oct 2, 2021

GameDungeon Oct 2, 2021

Timmmm Oct 2, 2021

Timmmm Oct 2, 2021

GameDungeon Oct 2, 2021

GameDungeon Oct 2, 2021

Timmmm Oct 2, 2021

GameDungeon Oct 2, 2021

Timmmm Oct 2, 2021

GameDungeon Oct 2, 2021

Timmmm Oct 2, 2021

Timmmm commented Oct 2, 2021

Timmmm commented Oct 2, 2021

GameDungeon commented Oct 2, 2021

Timmmm commented Oct 2, 2021

Darylgolden commented Jan 14, 2022

Darylgolden commented Jan 22, 2022

Timmmm commented Jan 22, 2022


		import numpy as np

		# TODO(types): colour has no type annotations.

Draft: Add type annotations to mobject.py #2129

Draft: Add type annotations to mobject.py #2129

Conversation

Timmmm commented Oct 1, 2021

Reviewer Checklist

Darylgolden commented Oct 2, 2021

GameDungeon commented Oct 2, 2021

GameDungeon commented Oct 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Timmmm commented Oct 2, 2021

Timmmm commented Oct 2, 2021

GameDungeon commented Oct 2, 2021

Timmmm commented Oct 2, 2021

Darylgolden commented Jan 14, 2022

Darylgolden commented Jan 22, 2022

Timmmm commented Jan 22, 2022