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

Rename InternalPoint3D to Point3D, Point3D to Point3DLike and other point-related type aliases #4027

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

Conversation

chopan050
Copy link
Contributor

Closes #4016

In my attempts to add typehints to subclasses of Mobjects, I've been repeatedly facing the following problem:

There are common methods like Mobject.get_center() which are typed to return a Point3D, where Point3D is a type alias defined as a union between a NumPy array and a tuple of 3 floats. However, the actual return type is always a NumPy array, which is more precisely described by the InternalPoint3D type alias which only represents that: a NumPy array.

The values returned from those methods are normally used as NumPy arrays: we add them with other arrays, multiply them, etc. However, MyPy complains about that, because Point3D is a union with a tuple, which does not support those operations, at least not in the way NumPy arrays do.

The most straightforward solution would be to simply typehint the returns as InternalPoint3D instead. However, InternalPoint3D is described as only for Manim internal use. It does not make much sense to use something "internal" as a return value for multiple functions and methods which we encourage Manim users to call in their code.

Since there are a lot of methods and functions returning NumPy arrays like that, I would say that the best approach would be to stop treating InternalPoint3D as only internal. IMO, this requires a name change. Therefore, I propose the following:

  • InternalPoint3D is renamed to Point3D: a NumPy array, which is returned by many functions and methods.
  • Point3D is renamed to Point3DLike: anything resembling a 3D point, either a NumPy array or a tuple[float, float, float]. Users should, in general, be able to pass Point3DLike arguments (anything resembling a 3D point) to their functions and expect Point3D (always a NumPy array) return values from them.

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

manim/mobject/geometry/arc.py Dismissed Show dismissed Hide dismissed
manim/utils/bezier.py Fixed Show fixed Hide fixed
manim/utils/bezier.py Fixed Show fixed Hide fixed
manim/utils/bezier.py Fixed Show fixed Hide fixed
manim/utils/bezier.py Fixed Show fixed Hide fixed
manim/utils/bezier.py Fixed Show fixed Hide fixed
manim/utils/bezier.py Dismissed Show dismissed Hide dismissed
manim/utils/bezier.py Dismissed Show dismissed Hide dismissed
Copy link
Member

@JasonGrace2282 JasonGrace2282 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't taken an in-depth look at the changes, but could you also update our docs on choosing typehints?
https://manimce--4027.org.readthedocs.build/en/4027/contributing/docs/types.html

@chopan050
Copy link
Contributor Author

chopan050 commented Nov 30, 2024

could you also update our docs on choosing typehints?

Done! Please, take a look and tell me what you think about it, @JasonGrace2282

@chopan050
Copy link
Contributor Author

.. warning::
This is not always true. For example, as of Manim 0.18.0, the direction
parameter of the :class:.Vector Mobject should be Point2D | Point3D,
as it can also accept tuple[float, float] and tuple[float, float, float].

To me, this indicates that we also need Vector2DLike, Vector3DLike and similar type aliases.
There are also methods like Mobject.rotate() which can actually accept hypothetical Vector3DLike arguments in their current state, methods like Mobject.shift() which could do that with a small rewriting (np.sum(vectors, axis=0) instead of reduce(op.add, vectors)) and functions like cross from space_ops.

Also, it's a bit weird that find_intersection from space_ops accepts Point3DLike_Array for p0s and p1s, but not Vector3DLike_Array for v0s and v1s.

I would like to discuss this more. If we agree on this, I could add those Vector...Like... type aliases, either in this PR or in a different one.

"""Wrap ``function``'s output inside a NumPy array."""
return np.asarray(function(t))

self.function = internal_parametric_function

Check warning

Code scanning / CodeQL

Overwriting attribute in super-class or sub-class Warning

Assignment overwrites attribute function, which was previously defined in subclass
FunctionGraph
.
Copy link
Member

@JasonGrace2282 JasonGrace2282 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making this PR!
Definitely looking forward to this change. Left a few comments, would appreciate if you could take a look when you find time.

manim/mobject/geometry/line.py Outdated Show resolved Hide resolved
manim/mobject/geometry/polygram.py Show resolved Hide resolved
manim/mobject/types/vectorized_mobject.py Outdated Show resolved Hide resolved
manim/mobject/types/vectorized_mobject.py Outdated Show resolved Hide resolved
manim/utils/bezier.py Outdated Show resolved Hide resolved
manim/utils/polylabel.py Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

Type aliases like Point3D and InternalPoint3D should probably be renamed
2 participants