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.

Sign up for GitHub

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

Jump to bottom

Adding docs for VectorScene and LinearTransformationScene and reimplementing method #3681

Closed
wants to merge 4 commits into from
Closed

Adding docs for VectorScene and LinearTransformationScene and reimplementing method #3681

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0639a49
I have changed the add_axes method and added the docs for this method…
SirJamesClarkMaxwell Apr 7, 2024
f262661
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Apr 8, 2024
cffac8d
removed the wrong changes
SirJamesClarkMaxwell Apr 8, 2024
390aa3a
Merge branch 'VectorScene-fixingBugs-docs' of https://github.com/SirJ…
SirJamesClarkMaxwell Apr 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 82 additions & 9 deletions manim/scene/vector_space_scene.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

__all__ = ["VectorScene", "LinearTransformationScene"]

from typing import Callable
from typing import Any, Callable

import numpy as np

Expand All @@ -14,7 +14,7 @@
from manim.mobject.graphing.coordinate_systems import Axes, NumberPlane
from manim.mobject.opengl.opengl_mobject import OpenGLMobject
from manim.mobject.text.tex_mobject import MathTex, Tex
from manim.utils.config_ops import update_dict_recursively
from manim.utils.config_ops import merge_dicts_recursively

from .. import config
from ..animation.animation import Animation
Expand Down Expand Up @@ -79,20 +79,90 @@ def add_plane(self, animate: bool = False, **kwargs):
self.add(plane)
return plane

def add_axes(self, animate: bool = False, color: bool = WHITE, **kwargs):
def add_axes(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why did I remove the color argument?

Because it is not working.
self.add_axes(color=RED) will not work, because the color of the VGroup is set before the axes creation.
I could add these lines:

if color:
    axes.set_color(color)

but I don't see the reason, the much more powerful would be the new solution when a user could use dictionaries to specify the exact all parameters in axes

self,
animate: bool = False,
animation: Animation = Create,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

animation, animation_kwargs and play_kwargs

Why must a user be limited to the Create animation? -> Solution: my changes
It would annoy me if I wanted to create a more advanced animation but I would have only Create animation available.

animation_kwargs: dict[str, Any] | None = {},
play_kwargs: dict[str, Any] | None = {},
axis_config: dict[str, Any] | None = {},
**axis_kwargs: dict[str, Any] | None,
Comment on lines +86 to +89
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
animation_kwargs: dict[str, Any] | None = {},
play_kwargs: dict[str, Any] | None = {},
axis_config: dict[str, Any] | None = {},
**axis_kwargs: dict[str, Any] | None,
animation_kwargs: dict[str, Any] = {},
play_kwargs: dict[str, Any] = {},
axis_config: dict[str, Any] = {},
**axis_kwargs: Any,

These are the correct typehints, as your code does not deal with the case of each parameter being None.
In addition to that, **kwargs is always typehinted as the type of the value of the dict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, I should identify the method of depreciation, remove them, and add examples for that, which will not be deprecated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would remove the following:

  1. add_plane
  2. add_axes
  3. add_vector
  4. position_x_coordinate
  5. position_y_coordinate
    and in LinearTransformationScene
    I would move the content of the add_vector (from the base class) into the body of the add_vector in the derived class.

If I understand, please let me know if I am wrong. Does the depreciation method mean deleting them from the source code? Right?

Copy link
Contributor

@JasonGrace2282 JasonGrace2282 Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

After taking a look at the source code again, I will say that it's my mistake. These methods shouldn't be deprecated.
Instead of adding lots of parameters, the user should simply pass animate=False and then animate it themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this mean I should create as many code examples as possible?
Please explain a little bit about self.moving_mobjects member variable? What is this responsible for?

Copy link
Contributor

@JasonGrace2282 JasonGrace2282 Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This means the implementation of the methods should be the same.
As for code examples, something like the following would suffice:

>>> from manim import LinearTransformationScene
>>> scene = LinearTransformationScene()
>>> scene.add_axes()
Axes(NumberLine, NumberLine)

):
"""
Adds a pair of Axes to the Scene.

Parameters
----------
animate
Whether or not to animate the addition of the axes through Create.
color
The color of the axes. Defaults to WHITE.
"""
axes = Axes(color=color, axis_config={"unit_size": 1})

animation
The animation used to add the axes.

animation_kwargs
Any valid keyword arguments of animation.

play_kwargs
Any valid keyword arguments of play.

axis_config
Any valid keyword arguments of Axes.

axis_kwargs
Any valid keyword arguments of Axes.

Returns
-------
Axes
The Axes object.

Examples
--------
.. manim:: AddingAxes
class AddingAxes(VectorScene):
def construct(self):
axis_config = {"include_tip": True, "unit_size": 2, "include_numbers": True}
animation = FadeIn
animation_kwargs = {"shift": LEFT}
play_kwargs = {"run_time": 1, "rate_func": linear}

axes = self.add_axes(
animate=True,
animation=animation,
animation_kwargs=animation_kwargs,
play_kwargs=play_kwargs,
axis_config=axis_config,
x_range=[0, 10, 1],
y_range=[-2, 6, 1],
x_length=5,
y_length=5,
)
self.play(axes.animate.shift(LEFT))
self.wait()

.. manim:: AddingAxes2
class AddingAxes2(VectorScene):
def construct(self):
axis_config = {"include_tip": True, "unit_size": 2, "include_numbers": True}
axes = self.add_axes(animate=True, axis_config=axis_config)
self.play(axes.animate.shift(LEFT))
self.wait()

"""
axis_kwargs = merge_dicts_recursively(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be marked in docs that there are default parameters for axis settings, and to change it the user have to pass it explicitly

{
"x_range": [-5, 5, 1],
"y_range": [-5, 5, 1],
"x_length": 5,
"y_length": 5,
},
axis_kwargs,
)
axis_config = merge_dicts_recursively({"unit_size": 1}, axis_config)

axes = Axes(**axis_kwargs, axis_config=axis_config)
if animate:
self.play(Create(axes))
self.play(animation(axes, **animation_kwargs), **play_kwargs)
self.add(axes)
return axes

Expand Down Expand Up @@ -1003,8 +1073,10 @@ def get_piece_movement(self, pieces: list | tuple | np.ndarray):
Animation
The animation of the movement.
"""
start = VGroup(*pieces)

start = VGroup(pieces)
target = VGroup(*(mob.target for mob in pieces))

# don't add empty VGroups
if self.leave_ghost_vectors and start.submobjects:
# start.copy() gives a VGroup of Vectors
Expand Down Expand Up @@ -1091,6 +1163,7 @@ def apply_matrix(self, matrix: np.ndarray | list | tuple, **kwargs):
**kwargs
Any valid keyword argument of self.apply_transposed_matrix()
"""

self.apply_transposed_matrix(np.array(matrix).T, **kwargs)

def apply_inverse(self, matrix: np.ndarray | list | tuple, **kwargs):
Expand Down
Loading