Improve @ operator to work on different coordinate systems#3879
Improve @ operator to work on different coordinate systems#3879JasonGrace2282 wants to merge 6 commits intoManimCommunity:mainfrom
@ operator to work on different coordinate systems#3879Conversation
JasonGrace2282
force-pushed
the
improve-matmul
branch
from
501b16c to
ccdd84c
Compare
chopan050
left a comment
chopan050
left a comment
There was a problem hiding this comment.
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!
| 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.
| def coords_to_point(self, *coords: float): | |
| def coords_to_point(self, *coords: float) -> Point3D: |
| return T_label_group | ||
|
|
||
| def __matmul__(self, coord: Point3D | Mobject): | ||
| def __matmul__(self, coord: Iterable[float] | Mobject): |
There was a problem hiding this comment.
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: |
| coord = coord.get_center() | ||
| return self.coords_to_point(*coord) | ||
|
|
||
| def __rmatmul__(self, point: Point3D): |
There was a problem hiding this comment.
Maybe the return type should be something like this?
| def __rmatmul__(self, point: Point3D): | |
| def __rmatmul__(self, point: Point3D) -> Point2D | 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.
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.
| def __rmatmul__(self, point: Point3D): | |
| def __rmatmul__(self, point: Point3D) -> complex: |
|
|
||
| return MathTex(string, font_size=font_size, **kwargs) | ||
|
|
||
| def __matmul__(self, coord: Point2D): |
There was a problem hiding this comment.
| 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.
| def __rmatmul__(self, point: Point2D): | |
| def __rmatmul__(self, point: Point3D) -> Point2D: |
|
After thinking about it for a while, I believe that we can merge this as is currently. Some of the changes I proposed need discussion, and we must later create a follow-up PR which completes |
3 participants
Pretty much getting it to work on
If I missed a coordinate system please let me know!