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

Improve @ operator to work on different coordinate systems #3879

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

Conversation

JasonGrace2282
Copy link
Member

Pretty much getting it to work on

  • PolarPlane
  • ComplexPlane
  • etc.

If I missed a coordinate system please let me know!

@JasonGrace2282 JasonGrace2282 added the enhancement Additions and improvements in general label Jul 22, 2024
@JasonGrace2282 JasonGrace2282 added this to the v0.19.0 milestone Jul 22, 2024
Copy link
Contributor

@chopan050 chopan050 left a 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):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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):
Copy link
Contributor

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).

Suggested change
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):
Copy link
Contributor

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?

Suggested change
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):
Copy link
Contributor

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.

Suggested change
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):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def __rmatmul__(self, point: Point2D):
def __rmatmul__(self, point: Point3D) -> Point2D:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Additions and improvements in general
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

2 participants