ronantakizawa/codereview-bench huggingface dataset:
Repository: Lightning-AI/pytorch-lightning
File: pytorch_lightning/loops/utilities.py (Python)

Code before:
    loss = None
    hiddens = None

    # handle dict return
    if isinstance(training_step_output, dict):
        # this should not modify the `training_step_output`, as the user could be using it after `training_step_end`
        loss = training_step_output.get("loss")
        hiddens = training_step_output.get("hiddens")
        # detach hiddens to avoid `RuntimeError: Trying to backward through the graph a second time`
        hiddens = apply_to_collection(hiddens, torch.Tensor, lambda t: t.detach())
        # use the setter instead of `dict.update` because it calls `detach` on the tensor items
        results.extra = {k: v for k, v in training_step_output.items() if k not in ("loss", "hiddens")}

    # handle scalar return
    elif isinstance(training_step_output, torch.Tensor):
        loss = training_step_output

    # map to results under the hood
    results.minimize = loss

    if trainer.move_metrics_to_cpu:
        results.cpu()
    return results, hiddens


def _prepare_dataloader_iter(dataloader_iter: Iterator, batch_idx: int) -> Iterator:
    """Attach the dataloader"""
    if not isinstance(dataloader_iter, DataLoaderIterDataFetcher):
        dataloader_iter = enumerate(dataloader_iter, batch_idx)
    # restore iteration
    return dataloader_iter


Review comment (refactor):
How much do we actually need to expose the "AbstractDataFetcher" here versus just an iterator? It seems to me that we're really not adding much value by exposing a new interface here. 

The custom logic below really should be part of the iterator implementation itself as i comment below.

Code after:
    loss = None
    hiddens = None

    # handle dict return
    if isinstance(training_step_output, dict):
        # this should not modify the `training_step_output`, as the user could be using it after `training_step_end`
        loss = training_step_output.get("loss")
        hiddens = training_step_output.get("hiddens")
        # detach hiddens to avoid `RuntimeError: Trying to backward through the graph a second time`
        hiddens = apply_to_collection(hiddens, torch.Tensor, lambda t: t.detach())
        # use the setter instead of `dict.update` because it calls `detach` on the tensor items
        results.extra = {k: v for k, v in training_step_output.items() if k not in ("loss", "hiddens")}

    # handle scalar return
    elif isinstance(training_step_output, torch.Tensor):
        loss = training_step_output

    # map to results under the hood
    results.minimize = loss

    if trainer.move_metrics_to_cpu:
        results.cpu()
    return results, hiddens


def _prepare_dataloader_iter(data_fetcher: AbstractDataFetcher, batch_idx: int) -> Iterator:
    """Attach the dataloader"""
    if not isinstance(data_fetcher, DataLoaderIterDataFetcher):
        # restore iteration
        dataloader_iter = enumerate(data_fetcher, batch_idx)
    else:
        dataloader_iter = iter(data_fetcher)
    return dataloader_iter
Repository: Lightning-AI/pytorch-lightning
File: pytorch_lightning/lite/wrappers.py (Python)

Code before:
    for subclass in _get_all_subclasses(DataLoader):
        _enable_class(subclass)
    yield
    for subclass in _get_all_subclasses(DataLoader):
        _disable_class(subclass)


class _LiteDataLoader:
    def __init__(self, dataloader: DataLoader, device: Optional[torch.device] = None) -> None:
        """The LiteDataLoader is a wrapper for the :class:`~torch.utils.data.DataLoader`. It moves the data to the
        device automatically if the device is specified.

        Args:
            dataloader: The dataloader to wrap
            device: The device to which the data should be moved. By default the device is `None` and no data
                transfers will be made (identical behavior as :class:`~torch.utils.data.DataLoader`).
        """
        super().__init__()
        self.__dict__.update(dataloader.__dict__)
        self._dataloader = dataloader
        self._device = device

    @property
    def device(self) -> Optional[torch.device]:
        return self._device

    def __len__(self) -> int:
        return len(self._dataloader)

    def __iter__(self) -> Union[Iterator[Any], Generator[Any, None, None]]:
        iterator = iter(self._dataloader)
        if self._device is None:
            yield from iterator

        for item in iterator:
            yield move_data_to_device(item, self._device)


Review comment (suggestion):
returning inf here is a strong assumption. LiteDataLoader should wrap the underlying dataloader and there is currently no reason why we would change the behavior of ``len``.

Code after:
    for subclass in _get_all_subclasses(DataLoader):
        _enable_class(subclass)
    yield
    for subclass in _get_all_subclasses(DataLoader):
        _disable_class(subclass)


class _LiteDataLoader:
    def __init__(self, dataloader: DataLoader, device: Optional[torch.device] = None) -> None:
        """The LiteDataLoader is a wrapper for the :class:`~torch.utils.data.DataLoader`. It moves the data to the
        device automatically if the device is specified.

        Args:
            dataloader: The dataloader to wrap
            device: The device to which the data should be moved. By default the device is `None` and no data
                transfers will be made (identical behavior as :class:`~torch.utils.data.DataLoader`).
        """
        self.__dict__.update(dataloader.__dict__)
        self._dataloader = dataloader
        self._device = device

    @property
    def device(self) -> Optional[torch.device]:
        return self._device

    def __len__(self) -> int:
        return len(self._dataloader)

    def __iter__(self) -> Union[Iterator[Any], Generator[Any, None, None]]:
        iterator = iter(self._dataloader)
        if self._device is None:
            yield from iterator

        for item in iterator:
            yield move_data_to_device(item, self._device)
Repository: Lightning-AI/pytorch-lightning
File: src/lightning/pytorch/profilers/advanced.py (Python)

Code before:
        with (
            tempfile.TemporaryDirectory(prefix="test", suffix=str(rank_zero_only.rank), dir=os.getcwd()) as tmp_dir,
            dst_fs.open(dst_filepath, "wb") as dst_file,
        ):
            src_filepath = os.path.join(tmp_dir, "tmp.prof")
            profile.dump_stats(src_filepath)
            src_fs = get_filesystem(src_filepath)
            with src_fs.open(src_filepath, "rb") as src_file:
                dst_file.write(src_file.read())

    @override
    def summary(self) -> str:
        recorded_stats = {}
        for action_name, pr in self.profiled_actions.items():
            if self.dump_stats:
                self._dump_stats(action_name, pr)
            s = io.StringIO()
            ps = pstats.Stats(pr, stream=s).strip_dirs().sort_stats("cumulative")
            ps.print_stats(self.line_count_restriction)
            recorded_stats[action_name] = s.getvalue()
        return self._stats_to_str(recorded_stats)

    @override
    def teardown(self, stage: Optional[str]) -> None:
        super().teardown(stage=stage)
        self.profiled_actions = defaultdict(cProfile.Profile)

    def __reduce__(self) -> tuple:
        # avoids `TypeError: cannot pickle 'cProfile.Profile' object`
        return (
            self.__class__,
            (),
            {"dirpath": self.dirpath, "filename": self.filename, "line_count_restriction": self.line_count_restriction},
        )


Review comment (suggestion):
```suggestion
        self.profiled_actions.clear()
```

Code after:
        with (
            tempfile.TemporaryDirectory(prefix="test", suffix=str(rank_zero_only.rank), dir=os.getcwd()) as tmp_dir,
            dst_fs.open(dst_filepath, "wb") as dst_file,
        ):
            src_filepath = os.path.join(tmp_dir, "tmp.prof")
            profile.dump_stats(src_filepath)
            src_fs = get_filesystem(src_filepath)
            with src_fs.open(src_filepath, "rb") as src_file:
                dst_file.write(src_file.read())

    @override
    def summary(self) -> str:
        recorded_stats = {}
        for action_name, pr in self.profiled_actions.items():
            if self.dump_stats:
                self._dump_stats(action_name, pr)
            s = io.StringIO()
            ps = pstats.Stats(pr, stream=s).strip_dirs().sort_stats("cumulative")
            ps.print_stats(self.line_count_restriction)
            recorded_stats[action_name] = s.getvalue()
        return self._stats_to_str(recorded_stats)

    @override
    def teardown(self, stage: Optional[str]) -> None:
        super().teardown(stage=stage)
        self.profiled_actions.clear()

    def __reduce__(self) -> tuple:
        # avoids `TypeError: cannot pickle 'cProfile.Profile' object`
        return (
            self.__class__,
            (),
            {"dirpath": self.dirpath, "filename": self.filename, "line_count_restriction": self.line_count_restriction},
        )
Repository: Lightning-AI/pytorch-lightning
File: src/lightning/fabric/utilities/device_parser.py (Python)

Code before:


def _check_data_type(device_ids: object) -> None:
    """Checks that the device_ids argument is one of the following: int, string, or sequence of integers.

    Args:
        device_ids: gpus/tpu_cores parameter as passed to the Trainer

    Raises:
        TypeError:
            If ``device_ids`` of GPU/TPUs aren't ``int``, ``str`` or sequence of ``int```

    """
    msg = "Device IDs (GPU/TPU) must be an int, a string, a sequence of ints, but you passed"
    if device_ids is None:
        raise TypeError(f"{msg} None")
    if isinstance(device_ids, (MutableSequence, tuple)):
        for id_ in device_ids:
            id_type = type(id_)  # because `isinstance(False, int)` -> True
            if id_type is not int:
                raise TypeError(f"{msg} a sequence of {type(id_).__name__}.")
    elif type(device_ids) not in (int, str):
        raise TypeError(f"{msg} {device_ids!r}.")


def _select_auto_accelerator_fabric() -> str:
    """Choose the accelerator type (str) based on availability."""
    from lightning.fabric.accelerators.cuda import CUDAAccelerator
    from lightning.fabric.accelerators.mps import MPSAccelerator
    from lightning.fabric.accelerators.xla import XLAAccelerator

    if XLAAccelerator.is_available():
        return "tpu"
    if MPSAccelerator.is_available():
        return "mps"
    if CUDAAccelerator.is_available():
        return "cuda"
    return "cpu"


Review comment (suggestion):
```suggestion
def _select_auto_accelerator() -> str:
```

Code after:


def _check_data_type(device_ids: object) -> None:
    """Checks that the device_ids argument is one of the following: int, string, or sequence of integers.

    Args:
        device_ids: gpus/tpu_cores parameter as passed to the Trainer

    Raises:
        TypeError:
            If ``device_ids`` of GPU/TPUs aren't ``int``, ``str`` or sequence of ``int```

    """
    msg = "Device IDs (GPU/TPU) must be an int, a string, a sequence of ints, but you passed"
    if device_ids is None:
        raise TypeError(f"{msg} None")
    if isinstance(device_ids, (MutableSequence, tuple)):
        for id_ in device_ids:
            id_type = type(id_)  # because `isinstance(False, int)` -> True
            if id_type is not int:
                raise TypeError(f"{msg} a sequence of {type(id_).__name__}.")
    elif type(device_ids) not in (int, str):
        raise TypeError(f"{msg} {device_ids!r}.")


def _select_auto_accelerator() -> str:
    """Choose the accelerator type (str) based on availability."""
    from lightning.fabric.accelerators.cuda import CUDAAccelerator
    from lightning.fabric.accelerators.mps import MPSAccelerator
    from lightning.fabric.accelerators.xla import XLAAccelerator

    if XLAAccelerator.is_available():
        return "tpu"
    if MPSAccelerator.is_available():
        return "mps"
    if CUDAAccelerator.is_available():
        return "cuda"
    return "cpu"
Repository: ManimCommunity/manim
File: manim/mobject/mobject.py (Python)

Code before:
            :code:`self`

        See Also
        --------
        :meth:`~Mobject.add`

        """
        for mobject in mobjects:
            if mobject in self.submobjects:
                self.submobjects.remove(mobject)
        return self

    def get_array_attrs(self):
        return ["points"]

    # This does not seem to be in use any more:
    # def digest_mobject_attrs(self):
    #     """
    #     Ensures all attributes which are mobjects are included
    #     in the submobjects list.
    #     """
    #     mobject_attrs = [
    #         x for x in list(self.__dict__.values()) if isinstance(x, Mobject)
    #     ]
    #     self.submobjects = list_update(self.submobjects, mobject_attrs)
    #     return self

    def apply_over_attr_arrays(self, func):
        for attr in self.get_array_attrs():
            setattr(self, attr, func(getattr(self, attr)))
        return self

    # Displaying

    def get_image(self, camera=None):
        if camera is None:
            from ..camera.camera import Camera

            camera = Camera()
        camera.capture_mobject(self)
        return camera.get_image()

    def show(self, camera=None):
        self.get_image(camera=camera).show()

    def save_image(self, name=None):
        self.get_image().save(
            Path(config.get_dir("video_dir")).joinpath((name or str(self)) + ".png")
        )

    def copy(self):

Review comment (suggestion):
Issue: please don't add commented out code to the codebase. If you mean to delete the code, just delete it :slightly_smiling_face: 
```suggestion
```

Code after:
        mobjects : :class:`Mobject`
            The mobjects to remove.

        Returns
        -------
        :class:`Mobject`
            :code:`self`

        See Also
        --------
        :meth:`~Mobject.add`

        """
        for mobject in mobjects:
            if mobject in self.submobjects:
                self.submobjects.remove(mobject)
        return self

    def get_array_attrs(self):
        return ["points"]

    def apply_over_attr_arrays(self, func):
        for attr in self.get_array_attrs():
            setattr(self, attr, func(getattr(self, attr)))
        return self

    # Displaying

    def get_image(self, camera=None):
        if camera is None:
            from ..camera.camera import Camera

            camera = Camera()
        camera.capture_mobject(self)
        return camera.get_image()

    def show(self, camera=None):
        self.get_image(camera=camera).show()

    def save_image(self, name=None):
        self.get_image().save(
            Path(config.get_dir("video_dir")).joinpath((name or str(self)) + ".png")
        )

    def copy(self):
        return copy.deepcopy(self)

    def generate_target(self, use_deepcopy=False):
        self.target = None  # Prevent unbounded linear recursion
        if use_deepcopy:
            self.target = copy.deepcopy(self)
Repository: ManimCommunity/manim
File: tests/test_scene_rendering/test_cli_flags.py (Python)

Code before:
        "manim",
        simple_scenes_path,
        scene_name,
        "-ql",
        "-s",
        "--media_dir",
        str(tmp_path),
        "--custom_folders",
    ]
    out, err, exit_code = capture(command)
    assert exit_code == 0, err

    exists = (tmp_path / "videos").exists()
    assert not exists, "--custom_folders produced a 'videos/' dir"

    exists = (tmp_path / "SquareToCircle.png").exists()
    assert exists, "--custom_folders did not produce the output file"


@pytest.mark.slow
def test_dash_as_filename(tmp_path):
    code = "class Test(Scene):\n    def construct(self):\n        self.add(Circle())\n        self.wait(1)"
    command = " ".join(
        [
            "echo",
            f'"{code}"',
            "|",
            "python",
            "-m",
            "manim",
            "-ql",
            "-s",
            "--media_dir",
            str(tmp_path),
            "-",
        ]
    )
    out, err, exit_code = capture(command, shell=True)
    assert exit_code == 0, err
    exists = (tmp_path / "images" / "-" / "Test.png").exists()
    assert exists, "Output was not found at expected location."


Review comment (suggestion):
*Pretty* sure this doesn't need to be put in quotation marks, so
```suggestion
            code,
```

Code after:
        "manim",
        simple_scenes_path,
        scene_name,
        "-ql",
        "-s",
        "--media_dir",
        str(tmp_path),
        "--custom_folders",
    ]
    out, err, exit_code = capture(command)
    assert exit_code == 0, err

    exists = (tmp_path / "videos").exists()
    assert not exists, "--custom_folders produced a 'videos/' dir"

    exists = (tmp_path / "SquareToCircle.png").exists()
    assert exists, "--custom_folders did not produce the output file"


@pytest.mark.slow
def test_dash_as_filename(tmp_path):
    code = "class Test(Scene):\n    def construct(self):\n        self.add(Circle())\n        self.wait(1)"
    command = [
        "python",
        "-m",
        "manim",
        "-ql",
        "-s",
        "--media_dir",
        str(tmp_path),
        "-",
    ]
    out, err, exit_code = capture(command, command_input=code)
    assert exit_code == 0, err
    exists = (tmp_path / "images" / "-" / "Test.png").exists()
    assert exists, out
Repository: ManimCommunity/manim
File: tests/test_scene_rendering/test_cli_flags.py (Python)

Code before:
        simple_scenes_path,
        scene_name,
        "-ql",
        "-s",
        "--media_dir",
        str(tmp_path),
        "--custom_folders",
    ]
    out, err, exit_code = capture(command)
    assert exit_code == 0, err

    exists = (tmp_path / "videos").exists()
    assert not exists, "--custom_folders produced a 'videos/' dir"

    exists = (tmp_path / "SquareToCircle.png").exists()
    assert exists, "--custom_folders did not produce the output file"


@pytest.mark.slow
def test_dash_as_filename(tmp_path):
    code = "class Test(Scene):\n    def construct(self):\n        self.add(Circle())\n        self.wait(1)"
    command = " ".join(
        [
            "echo",
            f'"{code}"',
            "|",
            "python",
            "-m",
            "manim",
            "-ql",
            "-s",
            "--media_dir",
            str(tmp_path),
            "-",
        ]
    )
    out, err, exit_code = capture(command, shell=True)
    assert exit_code == 0, err
    exists = (tmp_path / "images" / "-" / "Test.png").exists()
    assert exists, "Output was not found at expected location."


Review comment (suggestion):
I think this should be
```suggestion
            "||",
```

Code after:
        simple_scenes_path,
        scene_name,
        "-ql",
        "-s",
        "--media_dir",
        str(tmp_path),
        "--custom_folders",
    ]
    out, err, exit_code = capture(command)
    assert exit_code == 0, err

    exists = (tmp_path / "videos").exists()
    assert not exists, "--custom_folders produced a 'videos/' dir"

    exists = (tmp_path / "SquareToCircle.png").exists()
    assert exists, "--custom_folders did not produce the output file"


@pytest.mark.slow
def test_dash_as_filename(tmp_path):
    code = "class Test(Scene):\n    def construct(self):\n        self.add(Circle())\n        self.wait(1)"
    command = [
        "python",
        "-m",
        "manim",
        "-ql",
        "-s",
        "--media_dir",
        str(tmp_path),
        "-",
    ]
    out, err, exit_code = capture(command, command_input=code)
    assert exit_code == 0, err
    exists = (tmp_path / "images" / "-" / "Test.png").exists()
    assert exists, out
Repository: ManimCommunity/manim
File: manim/mobject/functions.py (Python)

Code before:
            self.t_max,
            *(discontinuities - dt),
            *(discontinuities + dt),
        ]
        boundary_times.sort()
        for t1, t2 in zip(boundary_times[0::2], boundary_times[1::2]):
            t_range = list(np.arange(t1, t2, self.get_step_size(t1)))
            if t_range[-1] != t2:
                t_range.append(t2)
            points = np.array([self.function(t) for t in t_range])
            valid_indices = np.apply_along_axis(np.all, 1, np.isfinite(points))
            points = points[valid_indices]
            if len(points) > 0:
                self.start_new_path(points[0])
                self.add_points_as_corners(points[1:])
        self.make_smooth()
        return self


class FunctionGraph(ParametricFunction):
    def __init__(self, function, x_min=None, x_max=None, **kwargs):
        self.x_min = x_min or -config["frame_x_radius"]
        self.x_max = x_max or config["frame_x_radius"]
        self.parametric_function = lambda t: np.array([t, function(t), 0])
        ParametricFunction.__init__(
            self, self.parametric_function, self.x_min, self.x_max, **kwargs
        )
        self.function = function

    def get_function(self):
        return self.function

    def get_point_from_function(self, x):
        return self.parametric_function(x)


Review comment (suggestion):
Is there a reason to remove `t_min=` and such besides not being strictly necessary? Because if not then I'd prefer for them to be added back for readability's sake

Code after:
            self.t_max,
            *(discontinuities - dt),
            *(discontinuities + dt),
        ]
        boundary_times.sort()
        for t1, t2 in zip(boundary_times[0::2], boundary_times[1::2]):
            t_range = list(np.arange(t1, t2, self.get_step_size(t1)))
            if t_range[-1] != t2:
                t_range.append(t2)
            points = np.array([self.function(t) for t in t_range])
            valid_indices = np.apply_along_axis(np.all, 1, np.isfinite(points))
            points = points[valid_indices]
            if len(points) > 0:
                self.start_new_path(points[0])
                self.add_points_as_corners(points[1:])
        self.make_smooth()
        return self


class FunctionGraph(ParametricFunction):
    def __init__(self, function, x_min=None, x_max=None, color=YELLOW, **kwargs):
        if x_min is None:
            x_min = -config["frame_x_radius"]
        if x_max is None:
            x_max = config["frame_x_radius"]
        self.x_min = x_min
        self.x_max = x_max
        self.parametric_function = lambda t: np.array([t, function(t), 0])
        ParametricFunction.__init__(
            self,
            self.parametric_function,
            t_min=self.x_min,
            t_max=self.x_max,
            color=color,
            **kwargs
        )
        self.function = function

    def get_function(self):
        return self.function

    def get_point_from_function(self, x):
        return self.parametric_function(x)