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

Add @ shorthand for CoordinateSystem methods coords_to_point (c2p) and point_to_coords (p2c) #3754

Merged
merged 12 commits into from
May 27, 2024

Conversation

JasonGrace2282
Copy link
Contributor

@JasonGrace2282 JasonGrace2282 commented May 9, 2024

Allows ax @ (1, 0, 0) for coords_to_point and (1, 0, 0) @ ax for point_to_coords

Inspired by @abul4fia

@behackl behackl added the new feature Enhancement specifically adding a new feature (feature request should be used for issues instead) label May 9, 2024
Copy link
Member

@behackl behackl left a comment

Choose a reason for hiding this comment

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

I like this a lot! 👍

It might make sense modify or include a new example in the example gallery that uses this notation.

manim/mobject/graphing/coordinate_systems.py Outdated Show resolved Hide resolved
def __matmul__(self, coord: Point3D | Mobject):
if isinstance(coord, Mobject):
coord = coord.get_center()
return self.coords_to_point(*coord)
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
return self.coords_to_point(*coord)
return self.coords_to_point(coord)

?
Probably a test would be nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with removing the * is that ax @ (1, 0, 0) then returns a 3x3 array, which I felt was kinda unintuitive.
As for the test, I already have a doctest in Axes. Is another test really necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the interest of getting this merged, I went ahead and added a test for the behavior.

JasonGrace2282 and others added 2 commits May 9, 2024 15:35
Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
@JasonGrace2282
Copy link
Contributor Author

I just added it to NumberLine and changed the CoordinateSystem example to use @ notation. In the future, maybe we should transition to that being the "standard" (although c2p can always exist for more complex expressions)

@chopan050 chopan050 changed the title Add shorthand for axes Add @ shorthand for CoordinateSystem methods coords_to_point (c2p) and point_to_coords (p2c) May 20, 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.

LGTM!

@Viicos
Copy link
Contributor

Viicos commented May 23, 2024

Will get back to the unresolved comment soon

manim/mobject/graphing/coordinate_systems.py Outdated Show resolved Hide resolved
Co-authored-by: Victorien <65306057+Viicos@users.noreply.github.com>
@JasonGrace2282 JasonGrace2282 enabled auto-merge (squash) May 27, 2024 17:18
@JasonGrace2282 JasonGrace2282 merged commit 90ae6ad into ManimCommunity:main May 27, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature Enhancement specifically adding a new feature (feature request should be used for issues instead)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants