-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Improve @
operator to work on different coordinate systems
#3879
base: main
Are you sure you want to change the base?
Conversation
501b16c
to
ccdd84c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, Jason! I just reviewed your proposal and tested it locally. It looks great!
I left a bunch of typing suggestions. Some of them require discussion, so let me know what you think!
@@ -147,7 +147,7 @@ def __init__( | |||
self.y_length = y_length | |||
self.num_sampled_graph_points_per_tick = 10 | |||
|
|||
def coords_to_point(self, *coords: ManimFloat): | |||
def coords_to_point(self, *coords: float): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def coords_to_point(self, *coords: float): | |
def coords_to_point(self, *coords: float) -> Point3D: |
@@ -1836,12 +1836,19 @@ def construct(self): | |||
|
|||
return T_label_group | |||
|
|||
def __matmul__(self, coord: Point3D | Mobject): | |||
def __matmul__(self, coord: Iterable[float] | Mobject): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a suggestion regarding the return type.
About the parameter type, I'm not sure. The Liskov substitution principle says that, when overriding methods, parameter types are contravariant and return types are covariant, which means that the parameter type cannot get more specific when overriding a method. It can only get broader or stay the same. The reverse is true for return types.
Maybe the parameter type could be Mobject
, because all of the coordinate systems accept a Mobject as an argument (except for ComplexPlane
, but that should be changed in another PR).
def __matmul__(self, coord: Iterable[float] | Mobject): | |
def __matmul__(self, coord: Iterable[float] | Mobject) -> Point3D: |
@@ -1836,12 +1836,19 @@ def construct(self): | |||
|
|||
return T_label_group | |||
|
|||
def __matmul__(self, coord: Point3D | Mobject): | |||
def __matmul__(self, coord: Iterable[float] | Mobject): | |||
if isinstance(coord, Mobject): | |||
coord = coord.get_center() | |||
return self.coords_to_point(*coord) | |||
|
|||
def __rmatmul__(self, point: Point3D): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the return type should be something like this?
def __rmatmul__(self, point: Point3D): | |
def __rmatmul__(self, point: Point3D) -> Point2D | Point3D | complex: |
@@ -3399,6 +3412,12 @@ def p2n(self, point: Point3D) -> complex: | |||
"""Abbreviation for :meth:`point_to_number`.""" | |||
return self.point_to_number(point) | |||
|
|||
def __matmul__(self, coord: float | complex): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that ComplexPlane.number_to_point()
, unlike the rest of coordinate systems, does not accept a Mobject
as its parameter. It should be changed, but let's do that in another PR.
def __matmul__(self, coord: float | complex): | |
def __matmul__(self, coord: float | complex) -> Point3D: |
def __matmul__(self, coord: float | complex): | ||
return self.number_to_point(coord) | ||
|
||
def __rmatmul__(self, point: Point3D): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def __rmatmul__(self, point: Point3D): | |
def __rmatmul__(self, point: Point3D) -> complex: |
@@ -3327,6 +3334,12 @@ def get_radian_label(self, number, font_size: float = 24, **kwargs: Any) -> Math | |||
|
|||
return MathTex(string, font_size=font_size, **kwargs) | |||
|
|||
def __matmul__(self, coord: Point2D): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def __matmul__(self, coord: Point2D): | |
def __matmul__(self, coord: Point2D) -> Point3D: |
def __matmul__(self, coord: Point2D): | ||
return self.polar_to_point(*coord) | ||
|
||
def __rmatmul__(self, point: Point2D): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def __rmatmul__(self, point: Point2D): | |
def __rmatmul__(self, point: Point3D) -> Point2D: |
Pretty much getting it to work on
If I missed a coordinate system please let me know!