Type Annotations: Fixing errors showing up in static type checking tool mypy #1241

cobordism · 2021-04-03T16:42:38Z

In light of some of the issues I raise in #1212, I have started a process of going through all mypy errors and trying to fix them.

This PR started by adding some Type Annotations to animations.py and a few to rate_functions and a few more places... I was following the mypy output on error at a time, skipping over harder ones and solving the simple ones.

Changelog / Overview

I have added and corrected typing hints in animations.py and rate_functions.py I have removed the use of np.clip in favour or min and max, so that a rate function is just float -> float as expected (and not Union[float, numpy.ndarray])
I have added a TypeVariable to Mobject.copy so that derived objects (like Group) are preserved by copying.
I continued into composition.py, transform.py, creation.py picking off the low hanging fruit. Mostly a change in type hints, but also a few minor refactors. Prime example: in the Write animation, self.run_time and self.lag_ratio were being set in the __init__ function and then set again in the parent __init__ function. I cleaned that up. Each variable should have only one level in the hierarchy where it is set/written during __init__.

I have changed the Wait() animation so that it has a Mobject attached to it - albeit an empty Mobject(). This allows us to tag the first argument to Animation() as Mobject instead of Optional[Mobject]. Keeping it as the latter would have required a myriad of checks of the form if animation.mobject is not None: animation.mobject.method(...) and that's not too nice.
There is a slight hickup in that a GLMobject is not (yet) a Mobject and using the Wait Animation in the GL context causes an error. I have found an ugly quick fix for this that I hope will become unnecessary soon as GLMobjects mature.

Motivation

This fixes a bunch of errors found by running the mypy type checker. See Issue #1212. I think we should have everything correctly typed, and no typing errors shown by mypy

Explanation for Changes

Animations will not crash when given a None type object. AnimationGroups can contain a Wait() animation.

Testing Status

All tests pass locally. No new tests added.

Further Comments

Try running mypy ./ before and after this PR to see the difference.

Before:
mypy Found 538 errors in 90 files

After:
mypy Found 491 errors in 90 files

... there is plenty more to clean up.

Checklist

I have read the Contributing Guidelines
I have written a descriptive PR title (see top of PR template for examples)
I have written a changelog entry for the PR or deem it unnecessary
My new functions/classes either have a docstring or are private
My new functions/classes have tests added and (optional) examples in the docs
My new documentation builds, looks correctly formatted, and adds no additional build warnings

Reviewer Checklist

The PR title is descriptive enough
The PR is labeled correctly
The changelog entry is completed if necessary
Newly added functions/classes either have a docstring or are private
Newly added functions/classes have tests added and (optional) examples in the docs
Newly added documentation builds, looks correctly formatted, and adds no additional build warnings

docs/source/contributing/reference/manim_directive.rst

naveen521kk · 2021-04-03T18:57:07Z

manim/animation/animation.py

        lag_ratio: float = DEFAULT_ANIMATION_LAG_RATIO,
        run_time: float = DEFAULT_ANIMATION_RUN_TIME,
-        rate_func: typing.Callable[[float, float], np.ndarray] = smooth,
+        rate_func: typing.Callable[[float], float] = smooth,


is typing.Callable Deprecated? https://docs.python.org/3/library/typing.html#typing.Callable

ah I see. but only since 3.9.

and it wasn't I who put it in there...

I suppose we could remove the import typing line, change this line to just Callable[[float].... and add from collections.abc import Callable.
Would that be the thing to do?

I tried adding it from collections.abc but of course our CI and our readthedocs failed because they try using python3.7 and 3.8. Let's keep importing from typing for now.

manim/animation/animation.py

manim/utils/rate_functions.py

cobordism · 2021-04-04T19:53:39Z

manim/animation/animation.py

+        self,
+        submobject: Mobject,
+        starting_submobject: Mobject,
+        # target_copy: Mobject,


Here is an issue that the signature of the interpolate_submobject method is different in Animation than in Transform. It needs cleaning up.. but if I add this line here uncommented, then a bunch of test fail.

cobordism · 2021-04-04T19:54:17Z

manim/animation/animation.py

        return self.remover

-    def is_dummy(self) -> bool:
-        return self.mobject is None


This is really not needed imo.

cobordism · 2021-04-04T19:57:41Z

manim/animation/transform.py

    def __init__(
        self, method: types.MethodType, *args, **kwargs
-    ) -> None:  # method typing? for args?
+    ) -> None:  # method typing (we want to specify Mobject method)? for args?


I have more information about this. types.methodType is an abstract method, and not a method bound to an object. We need the type for a bound method which is a Callable[...] and has a __self__ of type mobject.
However it looks like it might not be possible to do this correctly
https://stackoverflow.com/questions/64225522/python-typing-how-to-type-hint-a-variable-as-a-bound-method
https://stackoverflow.com/questions/58085648/mypy-type-annotation-for-a-bound-method

cobordism · 2021-04-06T18:49:12Z

I just realised that this PR was from my dev branch and I've been pushing too it constantly.
Shall I close this PR for now and recreate when it is ready again, or is it OK to stay open?

The theme of this PR is - fix many - but not all - errors coming from mypy.

for more information, see https://pre-commit.ci

jsonvillanueva

Looks mostly good although I have a few concerns:

manim/animation/animation.py

manim/animation/creation.py

manim/animation/transform.py

jsonvillanueva

LGTM

for more information, see https://pre-commit.ci

cobordism · 2021-04-15T21:53:48Z

I managed merge conflict in web editor. I hope it's OK. It's always hard to satisfy black and flake while working in web edit mode...

Anyway, since there is clearly so much more to do on type hints, I hope we can just merge this one now and then continue working on the issue...

kolibril13 · 2021-04-16T15:35:52Z

@cobordism : can you wait with merging until #1338 is a bit more formulated?
Then we can come back to this pr and know exactly how we want to have things consistent.

kolibril13 · 2021-04-16T15:36:55Z

It's always hard to satisfy black and flake while working in web edit mode...

Oh, you dont' have to satisfy black anymore, that does pre-commit.ci for you ;)
See. e.g. here: f1d28ee

manim/animation/creation.py

kolibril13 · 2021-04-16T16:39:17Z

manim/animation/animation.py

    def __init__(
        self,
-        mobject: Mobject,
+        mobject: Union[Mobject, None],


From https://docs.python.org/3/library/typing.html#typing.Union:

You can use Optional[X] as a shorthand for Union[X, None].

manim/animation/animation.py

manim/animation/composition.py

kolibril13 · 2021-04-16T16:45:45Z

manim/animation/creation.py

        return vmobject.get_color()

-    def get_all_mobjects(self) -> typing.List[typing.Union[Mobject, None]]:
+    def get_all_mobjects(self) -> List[Union[Mobject, None]]:


Also here : Optional[Mobject]

cobordism · 2021-04-16T17:03:52Z

@kolibril13

@cobordism : can you wait with merging until #1338 is a bit more formulated?
Then we can come back to this pr and know exactly how we want to have things consistent.

do you think that helps? Rather than merging and then continuing the work? After all, the type hints still require a lot of work overall... and at least this PR makes a start at reducing mypy errors.

On the whole, I'm somewhat ambivalent. It's up to you.

kolibril13 · 2021-04-16T17:57:55Z

and at least this PR makes a start at reducing mypy errors.

Let's merge it then.
I think waiting would only cause merge conflicts.

Aron Fischer and others added 4 commits April 1, 2021 23:41

Type hints in animations and rate_functions

802d36c

Type Signatures on most rate functions

689eb8d

A rate_function should just be float -> float

b9bf896

Merge branch 'master' into dev

e2226d9

naveen521kk reviewed Apr 3, 2021

View reviewed changes

Aron Fischer added 7 commits April 4, 2021 16:08

Merge branch 'master' of github.com:ManimCommunity/manim into dev

e65444a

black?

68a32f5

self.mobject is of type Union[mobject, None]

2bedb01

Typing Corrections in composition.py

b6f1a1b

chasing down more type errors for transformations

9e8f2e3

mobject.copy needs better typing

2ada98e

mobject.copy preserves derived type

a18f695

cobordism commented Apr 4, 2021

View reviewed changes

Aron Fischer added 4 commits April 6, 2021 19:36

Every Animation has a mobject, even Wait

9699d8b

Typing errors in creation.py

451d603

Typing errors in creation.py

eddf942

quick fix Wait to work with GL test

7a16d44

cobordism changed the title ~~corrected type hints in animations.py and rate_functions.py~~ Type Annotations: Fixing errors showing up in static type checking tool mypy Apr 6, 2021

Aron Fischer and others added 9 commits April 6, 2021 21:49

isort

98e183e

Merge branch 'master' into dev

bc54b95

Merge branch 'master' into dev

45155f8

import Callable from Typying to make test for python <3.9 work again

72c3033

Merge branch 'dev' of github.com:cobordism/manim into dev

838dbb2

import Callable from Typying to make test for python <3.9 work again

a20c81d

[pre-commit.ci] auto fixes from pre-commit.com hooks

6f8ac0d

for more information, see https://pre-commit.ci

to make flake8 happy

7f6ce48

Merge branch 'dev' of github.com:cobordism/manim into dev

c494180

Merge branch 'master' into dev

d3640d2

cobordism added the refactor Refactor or redesign of existing code label Apr 7, 2021

cobordism requested review from behackl and naveen521kk April 7, 2021 15:55

cobordism added 3 commits April 8, 2021 15:11

Merge branch 'master' into dev

27b78b6

Merge branch 'master' into dev

2700ea5

Merge branch 'master' into dev

c02f25a

jsonvillanueva reviewed Apr 15, 2021

View reviewed changes

manim/animation/animation.py Show resolved Hide resolved

manim/animation/creation.py Show resolved Hide resolved

manim/animation/transform.py Show resolved Hide resolved

jsonvillanueva added maintenance refactoring, typos, removing clutter/dead code, and other code quality improvements and removed refactor Refactor or redesign of existing code labels Apr 15, 2021

Merge branch 'master' into dev

4515e41

jsonvillanueva approved these changes Apr 15, 2021

View reviewed changes

jsonvillanueva and others added 3 commits April 15, 2021 12:26

Merge branch 'master' into dev

be6832e

Merge branch 'master' into dev

90eef30

[pre-commit.ci] auto fixes from pre-commit.com hooks

f1d28ee

for more information, see https://pre-commit.ci

Merge branch 'master' into dev

5e48caf

kolibril13 reviewed Apr 16, 2021

View reviewed changes

manim/animation/creation.py Show resolved Hide resolved

kolibril13 reviewed Apr 16, 2021

View reviewed changes

manim/animation/animation.py Show resolved Hide resolved

kolibril13 reviewed Apr 16, 2021

View reviewed changes

manim/animation/composition.py Show resolved Hide resolved

kolibril13 reviewed Apr 16, 2021

View reviewed changes

Merge branch 'master' into dev

98579c9

kolibril13 approved these changes Apr 16, 2021

View reviewed changes

cobordism merged commit a24c26c into ManimCommunity:master Apr 16, 2021

sparshg mentioned this pull request May 21, 2021

turn_animation_into_updater(anim) calls anim.update_config(**kwargs) which was removed #951

Closed

Type Annotations: Fixing errors showing up in static type checking tool mypy #1241

Type Annotations: Fixing errors showing up in static type checking tool mypy #1241

Uh oh!

Conversation

cobordism commented Apr 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog / Overview

Motivation

Explanation for Changes

Testing Status

Further Comments

Checklist

Reviewer Checklist

Uh oh!

Uh oh!

naveen521kk Apr 3, 2021

Choose a reason for hiding this comment

Uh oh!

cobordism Apr 3, 2021

Choose a reason for hiding this comment

Uh oh!

cobordism Apr 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cobordism Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cobordism Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

cobordism Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

cobordism Apr 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cobordism commented Apr 6, 2021

Uh oh!

jsonvillanueva left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsonvillanueva left a comment

Choose a reason for hiding this comment

Uh oh!

cobordism commented Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kolibril13 commented Apr 16, 2021

Uh oh!

kolibril13 commented Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kolibril13 Apr 16, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kolibril13 Apr 16, 2021

Choose a reason for hiding this comment

Uh oh!

cobordism commented Apr 16, 2021

Uh oh!

kolibril13 commented Apr 16, 2021

Uh oh!

Reviewers

Assignees

Labels

cobordism commented Apr 3, 2021 •

edited

Loading

cobordism Apr 3, 2021 •

edited

Loading

cobordism Apr 4, 2021 •

edited

Loading

cobordism commented Apr 15, 2021 •

edited

Loading

kolibril13 commented Apr 16, 2021 •

edited

Loading