Type annotation work in manim/mobject/geometry/ #3961
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The following four errors are removed with the commit. manim/mobject/geometry/labeled.py:63: error: Function is missing a type annotation for one or more arguments [no-untyped-def] manim/mobject/geometry/labeled.py:70: error: Incompatible default for argument "frame_fill_color" (default has type "None", argument has type "ManimColor | int | str | tuple[int, int, int] | tuple[float, float, float] | <6 more items>") [assignment] manim/mobject/geometry/labeled.py:82: error: Incompatible types in assignment (expression has type "MathTex | Text", variable has type "MathTex") [assignment] manim/mobject/geometry/labeled.py:150: error: Function is missing a type annotation for one or more arguments [no-untyped-def]
The following errors are removed with the commit.
manim/mobject/geometry/tips.py:115: error: Function is missing a type annotation for one or more arguments [no-untyped-def]
manim/mobject/geometry/tips.py:208: error: Function is missing a type annotation [no-untyped-def]
manim/mobject/geometry/tips.py:232: error: Function is missing a return type annotation [no-untyped-def]
manim/mobject/geometry/tips.py:245: error: Function is missing a type annotation for one or more arguments [no-untyped-def]
manim/mobject/geometry/tips.py:273: error: Function is missing a type annotation for one or more arguments [no-untyped-def]
manim/mobject/geometry/tips.py:282: error: Function is missing a type annotation for one or more arguments [no-untyped-def]
manim/mobject/geometry/tips.py:301: error: Function is missing a type annotation for one or more arguments [no-untyped-def]
manim/mobject/geometry/tips.py:310: error: Function is missing a type annotation for one or more arguments [no-untyped-def]
manim/mobject/geometry/tips.py:333: error: Function is missing a type annotation for one or more arguments [no-untyped-def]
The following errors are still present:
manim/mobject/geometry/tips.py:167: error: Unsupported left operand type for - ("tuple[float, float, float]") [operator]
manim/mobject/geometry/tips.py:167: note: Both left and right operands are unions
manim/mobject/geometry/tips.py:230: error: Argument 1 to "scale" of "Mobject" has incompatible type "floating[Any]"; expected "float" [arg-type]
TypeErrorCount: 1773
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Hey thanks for helping out with the typing!
Most likely, you shouldn't be using the |
|
Thanks for the tip about My plan is to focus on getting rid of most of these errors:
Now the type of errors reported by mypy for the files in mobject/geometry looks like shown below. |
|
A lot of those are quite difficult (mostly because Manim was not designed with typing in mind!) I would just For ones about returning If it's being annoying about numpy arrays being a If you need any more help or the PR is ready to review feel free to tag me :) P.S. this might be useful https://docs.manim.community/en/latest/contributing/docs/types.html |
The method is only used in one location where the return value is not stored.
|
Ok, status is that 21 errors are currently reported by mypy in the |
…he constructor." This reverts commit 80ae857.
|
For the first error, I'm not quite sure why mypy complains - does it think that You can The third looks like a bug in Manim ( assert self.path_arc is not Nonebefore creating the |
As I read the code, the Arrow class is derived from the Line class.
Fine.
Perfect, a solution is then found! Thanks for the input. |
|
@JasonGrace2282 When I earlier activated mypy on the CI workflow link, it found a lot of errors. Therefore it was deactivated again. The Code scanning results / CodeQL fails with a warning and a failure. The docs/readthedocs.org:manimce build also fails. |
|
Looking at the CI/CodeQL comments, I think it's fine to ignore them. About the docs build, that was recently fixed - you might have to resolve conflicts and then merge main into this branch. |
JasonGrace2282
requested changes
Oct 24, 2024
There was a problem hiding this comment.
Thanks so much for tackling this issue. I remember trying to type this back when I was first starting with Manim, and it's nice to see it done much more thoroughly and correctly.
I did leave some comments/questions, so it would be nice to work towards getting those worked out so that everyone can enjoy the benefits of IDE completion :)
Thanks for helping make Manim better ✨
Co-authored-by: Aarush Deshpande <[email protected]>
Thanks for all the suggestions. I have added a comment to each of the suggestions. In most cases I have implemented the suggestion. In the cases where I haven't implemented the suggestion a detailed comment explains why. |
|
@JasonGrace2282 It seems like the CI flow does not activate the tests, they are listed as "Expected -- Waiting for status to be reported". |
3 tasks
…ype_annotation_work
for more information, see https://pre-commit.ci
JasonGrace2282
approved these changes
Oct 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview: What does this pull request change?
The pull request is meant as a first step towards getting the automatic type annotations (mypy) to work on the codebase.
Motivation and Explanation: Why and how do your changes improve the library?
I found this issue listed as as "good first issue"
#2121
This lead to this issue
#3375
Further Information and Comments
The main issue that I am struggling with at the moment, are type errors like the one listed below
The issue is that mypy do not think that two Point3D objects can be subtracted from each other.
This operation is performed in multiple locations of the codebase, e.g. line 180 in arc.py
angles = cartesian_to_spherical(handle - anchor)Reviewer Checklist