ronantakizawa/codereview-bench huggingface dataset:
Repository: ManimCommunity/manim
File: manim/animation/specialized.py (Python)

Code before:
        self.initial_opacity = initial_opacity
        self.final_opacity = final_opacity
        self.initial_width = initial_width

        # create all the mobjects and move them to the focal point
        mobjects = VGroup(
            *[
                mobject.copy()
                .set_stroke(opacity=self.final_opacity)
                .move_to(self.focal_point)
                for _ in range(self.n_mobs)
            ]
        )

        for mobject in mobjects:
            mobject.save_state()
            mobject.set(width=self.initial_width)
            mobject.set_stroke(opacity=self.initial_opacity)

        # restore the mob to its original status
        # to create the effect of it growing from nothing
        animations = [Restore(mobject) for mobject in mobjects]
        super().__init__(
            *animations,
            run_time=run_time,
            lag_ratio=lag_raito,
            remover=remover,
            **kwargs
        )


Review comment (suggestion):
```suggestion
            lag_ratio=lag_ratio,
```

Code after:
        self.initial_width = initial_width

        anims = []

        # Works by saving the mob that is passed into the animation, scaling it to 0 (or the initial_width) and then restoring the original mob.
        if mobject.fill_opacity:
            fill_o = True
        else:
            fill_o = False

        for _ in range(self.n_mobs):
            mob = mobject.copy()

            if fill_o:
                mob.set_opacity(self.final_opacity)
            else:
                mob.set_stroke(opacity=self.final_opacity)

            mob.move_to(self.focal_point)
            mob.save_state()
            mob.set_width(self.initial_width)

            if fill_o:
                mob.set_opacity(self.initial_opacity)
            else:
                mob.set_stroke(opacity=self.initial_opacity)

            anims.append(Restore(mob, remover=remover))

        super().__init__(*anims, run_time=run_time, lag_ratio=lag_ratio, **kwargs)
Repository: ManimCommunity/manim
File: manim/animation/specialized.py (Python)

Code before:
__all__ = ["Broadcast"]

from typing import Sequence

from manim.animation.transform import Restore
from manim.mobject.mobject import Mobject

from ..constants import *
from ..mobject.types.vectorized_mobject import VGroup
from .composition import LaggedStart


class Broadcast(LaggedStart):
    """Broadcast a mobject starting from an ``initial_width``, up to the actual size of the mobject.

    Parameters
    ----------
    mobject
        The mobject to be broadcast.
    focal_point
        The point from which the mobjects emerge, by default ORIGIN.
    n_mobs
        The number of mobjects that originate from the focal point, by default 5.
    initial_opacity
        The starting stroke opacity of the mobjects emitted from the broadcast, by default 1.
    final_opacity
        The final stroke opacity of the mobjects emitted from the broadcast, by default 0.
    initial_width
        The initial width of the mobjects, by default 0.0.
    remover
        Whether the given mobject should be removed from the scene after the animation, by default True.
    lag_ratio
        The time between each iteration of the mobject, by default 0.2.
    run_time

Review comment (suggestion):
```suggestion
from ..mobject.types.vectorized_mobject import VMobject
```

Code after:
__all__ = ["Broadcast"]

from typing import Any, Sequence

from manim.animation.transform import Restore

from ..constants import *
from .composition import LaggedStart


class Broadcast(LaggedStart):
    """Broadcast a mobject starting from an ``initial_width``, up to the actual size of the mobject.

    Parameters
    ----------
    mobject
        The mobject to be broadcast.
    focal_point
        The center of the broadcast, by default ORIGIN.
    n_mobs
        The number of mobjects that emerge from the focal point, by default 5.
    initial_opacity
        The starting stroke opacity of the mobjects emitted from the broadcast, by default 1.
    final_opacity
        The final stroke opacity of the mobjects emitted from the broadcast, by default 0.
    initial_width
        The initial width of the mobjects, by default 0.0.
    remover
        Whether the mobjects should be removed from the scene after the animation, by default True.
    lag_ratio
        The time between each iteration of the mobject, by default 0.2.
    run_time
        The total duration of the animation, by default 3.
    kwargs
Repository: ManimCommunity/manim
File: manim/animation/specialized.py (Python)

Code before:

        # create all the mobjects and move them to the focal point
        mobjects = VGroup(
            *[
                mobject.copy()
                .set_stroke(opacity=self.final_opacity)
                .move_to(self.focal_point)
                for _ in range(self.n_mobs)
            ]
        )

        for mobject in mobjects:
            mobject.save_state()
            mobject.set(width=self.initial_width)
            mobject.set_stroke(opacity=self.initial_opacity)

        # restore the mob to its original status
        # to create the effect of it growing from nothing
        animations = [Restore(mobject) for mobject in mobjects]
        super().__init__(
            *animations,
            run_time=run_time,
            lag_ratio=lag_ratio,
            remover=remover,
            **kwargs
        )


Review comment (suggestion):
1. `mob.set(width=<width>)`  --->  `mob.set_width(<width>)`  since the former doesn't seem to work with the opengl stuff
2. `mob.set_stroke(opacity=<opacity>)`  --->  `mob.set_opacity(<opacity>)` since the passed `mobject` might have an initial non-zero `fill_opacity`.
3. `VGroup` is not really needed. Merged the three loops into one. 
```suggestion
        anims = []

        for _ in range(self.n_mobs):
            mob = mobject.copy()
            mob.set_opacity(self.final_opacity).move_to(self.focal_point)
            mob.save_state()
            mob.set_width(self.initial_width).set_opacity(self.initial_opacity)
            anims.append(Restore(mob))

        super().__init__(
            *anims, run_time=run_time, lag_ratio=lag_ratio, remover=remover, **kwargs
        )
```

Code after:

        # Works by saving the mob that is passed into the animation, scaling it to 0 (or the initial_width) and then restoring the original mob.
        if mobject.fill_opacity:
            fill_o = True
        else:
            fill_o = False

        for _ in range(self.n_mobs):
            mob = mobject.copy()

            if fill_o:
                mob.set_opacity(self.final_opacity)
            else:
                mob.set_stroke(opacity=self.final_opacity)

            mob.move_to(self.focal_point)
            mob.save_state()
            mob.set_width(self.initial_width)

            if fill_o:
                mob.set_opacity(self.initial_opacity)
            else:
                mob.set_stroke(opacity=self.initial_opacity)

            anims.append(Restore(mob, remover=remover))

        super().__init__(*anims, run_time=run_time, lag_ratio=lag_ratio, **kwargs)
Repository: ManimCommunity/manim
File: manim/animation/specialized.py (Python)

Code before:
        **kwargs
    ):
        self.focal_point = focal_point
        self.n_mobs = n_mobs
        self.initial_opacity = initial_opacity
        self.final_opacity = final_opacity
        self.initial_width = initial_width

        # create all the mobjects and move them to the focal point
        mobjects = VGroup(
            *[
                mobject.copy()
                .set_stroke(opacity=self.final_opacity)
                .move_to(self.focal_point)
                for _ in range(self.n_mobs)
            ]
        )

        for mobject in mobjects:
            mobject.save_state()
            mobject.set(width=self.initial_width)
            mobject.set_stroke(opacity=self.initial_opacity)

        # restore the mob to its original status
        # to create the effect of it growing from nothing
        animations = [Restore(mobject) for mobject in mobjects]
        super().__init__(
            *animations,
            run_time=run_time,
            lag_ratio=lag_ratio,
            remover=remover,
            **kwargs
        )


Review comment (suggestion):
```suggestion
        animations = [Restore(mobject, remover=remover) for mobject in mobjects]
```
I think setting the `remover` option here fixes the problem that the Mobjects don't remove themselves from the scene.

Code after:
        self.focal_point = focal_point
        self.n_mobs = n_mobs
        self.initial_opacity = initial_opacity
        self.final_opacity = final_opacity
        self.initial_width = initial_width

        anims = []

        # Works by saving the mob that is passed into the animation, scaling it to 0 (or the initial_width) and then restoring the original mob.
        if mobject.fill_opacity:
            fill_o = True
        else:
            fill_o = False

        for _ in range(self.n_mobs):
            mob = mobject.copy()

            if fill_o:
                mob.set_opacity(self.final_opacity)
            else:
                mob.set_stroke(opacity=self.final_opacity)

            mob.move_to(self.focal_point)
            mob.save_state()
            mob.set_width(self.initial_width)

            if fill_o:
                mob.set_opacity(self.initial_opacity)
            else:
                mob.set_stroke(opacity=self.initial_opacity)

            anims.append(Restore(mob, remover=remover))

        super().__init__(*anims, run_time=run_time, lag_ratio=lag_ratio, **kwargs)
Repository: ManimCommunity/manim
File: manim/mobject/three_dimensions.py (Python)

Code before:
"""Three-dimensional mobjects."""

__all__ = [
    "ThreeDVMobject",
    "ParametricSurface",
    "Sphere",
    "Dot3D",
    "Cube",
    "Prism",
    "Cone",
    "Arrow3D",
    "Cylinder",
    "Line3D",
    "Torus",
]

from typing import *

import numpy as np
from colour import Color

from manim.mobject.opengl_compatibility import ConvertToOpenGL

from ..constants import *
from ..mobject.geometry import Circle, Square
from ..mobject.mobject import *
from ..mobject.opengl_mobject import OpenGLMobject
from ..mobject.types.vectorized_mobject import VGroup, VMobject
from ..utils.color import *
from ..utils.deprecation import deprecated_params
from ..utils.iterables import tuplify
from ..utils.space_ops import normalize, z_to_vector


class ThreeDVMobject(VMobject, metaclass=ConvertToOpenGL):
    def __init__(self, shade_in_3d=True, **kwargs):
        super().__init__(shade_in_3d=shade_in_3d, **kwargs)


class ParametricSurface(VGroup, metaclass=ConvertToOpenGL):
    """Creates a Parametric Surface

    Parameters
    ----------
    func :

Review comment (suggestion):
nitpicky, but since `Color` is only used for type checking it should be under TYPE_CHECKING
```py
if TYPE_CHECKING:
    from colour import Color
```

Code after:
"""Three-dimensional mobjects."""

__all__ = [
    "ThreeDVMobject",
    "ParametricSurface",
    "Sphere",
    "Dot3D",
    "Cube",
    "Prism",
    "Cone",
    "Arrow3D",
    "Cylinder",
    "Line3D",
    "Torus",
]

from typing import *

import numpy as np

if TYPE_CHECKING:
    from colour import Color

from manim.mobject.opengl_compatibility import ConvertToOpenGL

from ..constants import *
from ..mobject.geometry import Circle, Square
from ..mobject.mobject import *
from ..mobject.opengl_mobject import OpenGLMobject
from ..mobject.types.vectorized_mobject import VGroup, VMobject
from ..utils.color import *
from ..utils.deprecation import deprecated_params
from ..utils.iterables import tuplify
from ..utils.space_ops import normalize, z_to_vector


class ThreeDVMobject(VMobject, metaclass=ConvertToOpenGL):
    def __init__(self, shade_in_3d=True, **kwargs):
        super().__init__(shade_in_3d=shade_in_3d, **kwargs)


class ParametricSurface(VGroup):
    """Creates a Parametric Surface

    Parameters
Repository: ManimCommunity/manim
File: manim/utils/space_ops.py (Python)

Code before:
    end0 = 0
    for end1 in ring_ends:
        after.extend(range(end0 + 1, end1))
        after.append(end0)
        end0 = end1

    # Find an ordering of indices walking around the polygon
    indices = []
    i = 0
    for _ in range(len(verts) + len(ring_ends) - 1):
        # starting = False
        if i in loop_connections:
            j = loop_connections[i]
            indices.extend([i, j])
            i = after[j]
        else:
            indices.append(i)
            i = after[i]
        if i == 0:
            break

    meta_indices = earcut(verts[indices, :2], [len(indices)])
    return [indices[mi] for mi in meta_indices]


def perpendicular_bisector(line: Sequence[Sequence[float]], norm_vector=OUT):
    p1 = line[0]
    p2 = line[1]
    direction = np.cross(p1 - p2, norm_vector)
    m = midpoint(p1, p2)
    return [m + direction, m - direction]


Review comment (suggestion):
Can you specify the return type, and add a doc string describing the input and outputs (explaining that the two lists are the endpoints of the line)

Code after:
    meta_indices = earcut(verts[indices, :2], [len(indices)])
    return [indices[mi] for mi in meta_indices]


def perpendicular_bisector(
    line: Sequence[np.ndarray], norm_vector=OUT
) -> Sequence[np.ndarray]:
    """Returns a list of two points that correspond
    to the ends of the perpendicular bisector of the
    two points given.

    Parameters
    ----------
    line
        a list of two numpy array points (corresponding
        to the ends of a line).
    norm_vector
        the vector perpendicular to both the line given
        and the perpendicular bisector.

    Returns
    -------
    list
        A list of two numpy array points that correspond
        to the ends of the perpendicular bisector
    """
    p1 = line[0]
    p2 = line[1]
    direction = np.cross(p1 - p2, norm_vector)
    m = midpoint(p1, p2)
    return [m + direction, m - direction]
Repository: ManimCommunity/manim
File: manim/animation/creation.py (Python)

Code before:
        Set True to have the animation start erasing from the last submobject first.

    Examples
    --------

    .. manim:: UnwriteReverseFalse

        class UnwriteReverseFalse(Scene):
            def construct(self):
                text = Tex("Alice and Bob").scale(3)
                self.add(text)
                self.play(Unwrite(text))

    .. manim :: UnwriteReverseTrue

        class UnwriteReverseTrue(Scene):
            def construct(self):
                text = Tex("Alice and Bob").scale(3)
                self.add(text)
                self.play(Unwrite(text,reverse=True))

    """

    def __init__(
        self,
        vmobject: VMobject,
        rate_func: Callable[[float], float] = linear,
        reverse: bool = False,
        **kwargs,
    ) -> None:

        run_time: Optional[float] = kwargs.pop("run_time", None)
        lag_ratio: Optional[float] = kwargs.pop("lag_ratio", None)
        run_time, lag_ratio = self._set_default_config_from_length(
            vmobject, run_time, lag_ratio
        )
        super().__init__(
            vmobject,
            run_time=run_time,
            lag_ratio=lag_ratio,
            rate_func=lambda t: -rate_func(t) + 1,
            reverse=reverse,
            **kwargs,
        )


class ShowIncreasingSubsets(Animation):
    """Show one submobject at a time, leaving all previous ones displayed on screen.

    Examples
    --------

Review comment (suggestion):
I haven't tried this locally yet, but I don't see how the behavior of `Unwrite` is preserved if the logic is flipped and moved to `Write`. Doesn't `Unwrite` now do the same as `Write`?

(I'm on mobile, so it's likely that I've missed something.)

Code after:

    Examples
    --------

    .. manim :: UnwriteReverseTrue

        class UnwriteReverseTrue(Scene):
            def construct(self):
                text = Tex("Alice and Bob").scale(3)
                self.add(text)
                self.play(Unwrite(text))

    .. manim:: UnwriteReverseFalse

        class UnwriteReverseFalse(Scene):
            def construct(self):
                text = Tex("Alice and Bob").scale(3)
                self.add(text)
                self.play(Unwrite(text, reverse=False))
    """

    def __init__(
        self,
        vmobject: VMobject,
        rate_func: Callable[[float], float] = linear,
        reverse: bool = True,
        **kwargs,
    ) -> None:

        run_time: Optional[float] = kwargs.pop("run_time", None)
        lag_ratio: Optional[float] = kwargs.pop("lag_ratio", None)
        run_time, lag_ratio = self._set_default_config_from_length(
            vmobject, run_time, lag_ratio
        )
        super().__init__(
            vmobject,
            run_time=run_time,
            lag_ratio=lag_ratio,
            rate_func=lambda t: -rate_func(t) + 1,
            reverse=reverse,
            **kwargs,
        )


class ShowIncreasingSubsets(Animation):
    """Show one submobject at a time, leaving all previous ones displayed on screen.

    Examples
    --------

    .. manim:: ShowIncreasingSubsetsScene
Repository: ManimCommunity/manim
File: tests/test_graphical_units/testing/_test_class_makers.py (Python)

Code before:
from typing import Callable, Type

from manim.renderer.cairo_renderer import CairoRenderer
from manim.scene.scene import Scene
from manim.scene.scene_file_writer import SceneFileWriter

from ._FramesTester import _FramesTester


def _make_test_scene_class(
    base_scene: Type[Scene],
    construct: Callable[[Scene], None],
    test_renderer,
) -> Type[Scene]:
    class _TestedScene(base_scene):
        def __init__(self, *args, **kwargs):
            super().__init__(renderer=test_renderer, *args, **kwargs)

        def construct(self):
            construct(self)

            # Manim hack to render the very last frame (normally the last frame is not the very end of the animation)
            self.update_to_time(self.get_run_time(self.animations))
            self.renderer.render(self, 1, self.moving_mobjects)

    return _TestedScene


def _make_test_renderer_class(from_renderer):
    # Just for heritance.
    class _TestRenderer(from_renderer):
        pass

    return _TestRenderer


class DummySceneFileWriter(SceneFileWriter):
    """Delegate of SceneFileWriter used to test the frames."""

    def __init__(self, renderer, scene_name, **kwargs):
        super().__init__(renderer, scene_name, **kwargs)
        self.i = 0

    def init_output_directories(self, scene_name):
        pass

Review comment (suggestion):
Just to avoid confusion, do you think it would make sense to rename the `construct` argument that is passed to `_make_test_scnee_class` to something like `construct_test`? Then these two lines would read
```py
def construct(self):
    construct_test(self)
```
which has less potential for confusion, I feel.

Code after:
from typing import Callable, Type

from manim.renderer.cairo_renderer import CairoRenderer
from manim.scene.scene import Scene
from manim.scene.scene_file_writer import SceneFileWriter

from ._frames_testers import _FramesTester


def _make_test_scene_class(
    base_scene: Type[Scene],
    construct_test: Callable[[Scene], None],
    test_renderer,
) -> Type[Scene]:
    class _TestedScene(base_scene):
        def __init__(self, *args, **kwargs):
            super().__init__(renderer=test_renderer, *args, **kwargs)

        def construct(self):
            construct_test(self)

            # Manim hack to render the very last frame (normally the last frame is not the very end of the animation)
            self.update_to_time(self.get_run_time(self.animations))
            self.renderer.render(self, 1, self.moving_mobjects)

    return _TestedScene


def _make_test_renderer_class(from_renderer):
    # Just for inheritance.
    class _TestRenderer(from_renderer):
        pass

    return _TestRenderer


class DummySceneFileWriter(SceneFileWriter):
    """Delegate of SceneFileWriter used to test the frames."""

    def __init__(self, renderer, scene_name, **kwargs):
        super().__init__(renderer, scene_name, **kwargs)
        self.i = 0

    def init_output_directories(self, scene_name):
        pass
Repository: ManimCommunity/manim
File: tests/test_graphical_units/testing/_test_class_makers.py (Python)

Code before:
from manim.scene.scene_file_writer import SceneFileWriter

from ._FramesTester import _FramesTester


def _make_test_scene_class(
    base_scene: Type[Scene],
    construct: Callable[[Scene], None],
    test_renderer,
) -> Type[Scene]:
    class _TestedScene(base_scene):
        def __init__(self, *args, **kwargs):
            super().__init__(renderer=test_renderer, *args, **kwargs)

        def construct(self):
            construct(self)

            # Manim hack to render the very last frame (normally the last frame is not the very end of the animation)
            self.update_to_time(self.get_run_time(self.animations))
            self.renderer.render(self, 1, self.moving_mobjects)

    return _TestedScene


def _make_test_renderer_class(from_renderer):
    # Just for heritance.
    class _TestRenderer(from_renderer):
        pass

    return _TestRenderer


class DummySceneFileWriter(SceneFileWriter):
    """Delegate of SceneFileWriter used to test the frames."""

    def __init__(self, renderer, scene_name, **kwargs):
        super().__init__(renderer, scene_name, **kwargs)
        self.i = 0

    def init_output_directories(self, scene_name):
        pass

    def add_partial_movie_file(self, hash_animation):
        pass

    def begin_animation(self, allow_write=True):
        pass

    def end_animation(self, allow_write):
        pass


Review comment (suggestion):
Typo (?)

```suggestion
    # Just for inheritance.
```

Code after:
from manim.scene.scene_file_writer import SceneFileWriter

from ._frames_testers import _FramesTester


def _make_test_scene_class(
    base_scene: Type[Scene],
    construct_test: Callable[[Scene], None],
    test_renderer,
) -> Type[Scene]:
    class _TestedScene(base_scene):
        def __init__(self, *args, **kwargs):
            super().__init__(renderer=test_renderer, *args, **kwargs)

        def construct(self):
            construct_test(self)

            # Manim hack to render the very last frame (normally the last frame is not the very end of the animation)
            self.update_to_time(self.get_run_time(self.animations))
            self.renderer.render(self, 1, self.moving_mobjects)

    return _TestedScene


def _make_test_renderer_class(from_renderer):
    # Just for inheritance.
    class _TestRenderer(from_renderer):
        pass

    return _TestRenderer


class DummySceneFileWriter(SceneFileWriter):
    """Delegate of SceneFileWriter used to test the frames."""

    def __init__(self, renderer, scene_name, **kwargs):
        super().__init__(renderer, scene_name, **kwargs)
        self.i = 0

    def init_output_directories(self, scene_name):
        pass

    def add_partial_movie_file(self, hash_animation):
        pass

    def begin_animation(self, allow_write=True):
        pass

    def end_animation(self, allow_write):
        pass