ronantakizawa/codereview-bench huggingface dataset:
Repository: FreeCAD/FreeCAD
File: src/Mod/BIM/ArchWindow.py (Python)

Code before:
            )
        )
        self.field6.setText(QtGui.QApplication.translate("Arch", "Get Selected Edge", None))
        self.field6.setToolTip(
            QtGui.QApplication.translate("Arch", "Press to retrieve the selected edge", None)
        )
        self.invertOpeningButton.setText(
            QtGui.QApplication.translate("Arch", "Invert Opening Direction", None)
        )
        self.invertHingeButton.setText(
            QtGui.QApplication.translate("Arch", "Invert Hinge Position", None)
        )
        for i in range(len(WindowPartTypes)):
            self.field2.setItemText(
                i, QtGui.QApplication.translate("Arch", WindowPartTypes[i], None)
            )
        for i in range(len(WindowOpeningModes)):
            self.field7.setItemText(
                i, QtGui.QApplication.translate("Arch", WindowOpeningModes[i], None)
            )

    def accept(self):
        if self.obj:
            self.obj.Width = self.widthWidget.property("rawValue")
            self.obj.Height = self.heightWidget.property("rawValue")
            self.obj.Opening = int(self.openingWidget.property("rawValue"))
        self.basepanel.obj = self.obj
        return self.basepanel.accept()

    def invertOpening(self):

        if self.obj:
            self.obj.ViewObject.Proxy.invertOpening()

    def invertHinge(self):

        if self.obj:
            self.obj.ViewObject.Proxy.invertHinge()


Review comment (suggestion):
```suggestion
            self.obj.Opening = round(self.openingWidget.property("rawValue"))
```
`int` will floor the value.

Code after:
        self.field6.setText(QtGui.QApplication.translate("Arch", "Get Selected Edge", None))
        self.field6.setToolTip(
            QtGui.QApplication.translate("Arch", "Press to retrieve the selected edge", None)
        )
        self.invertOpeningButton.setText(
            QtGui.QApplication.translate("Arch", "Invert Opening Direction", None)
        )
        self.invertHingeButton.setText(
            QtGui.QApplication.translate("Arch", "Invert Hinge Position", None)
        )
        for i in range(len(WindowPartTypes)):
            self.field2.setItemText(
                i, QtGui.QApplication.translate("Arch", WindowPartTypes[i], None)
            )
        for i in range(len(WindowOpeningModes)):
            self.field7.setItemText(
                i, QtGui.QApplication.translate("Arch", WindowOpeningModes[i], None)
            )

    def accept(self):
        if self.obj:
            self.obj.Width = self.widthWidget.property("value")
            self.obj.Height = self.heightWidget.property("value")
            self.obj.Opening = self.openingWidget.property("value")
        self.basepanel.obj = self.obj
        return self.basepanel.accept()

    def invertOpening(self):

        if self.obj:
            self.obj.ViewObject.Proxy.invertOpening()

    def invertHinge(self):

        if self.obj:
            self.obj.ViewObject.Proxy.invertHinge()
Repository: Genesis-Embodied-AI/Genesis
File: tests/test_rigid_physics.py (Python)

Code before:
            pos=(0.0, 0.0, 0.8),
        ),
        surface=gs.surfaces.Default(
            color=(0.5, 0.7, 0.9, 1.0),
        ),
    )
    scene.build()

    ball.set_dofs_velocity(np.random.rand(ball.n_dofs) * 0.8)
    for i in range(1500):
        scene.step()

    qvel = scene.sim.rigid_solver.dofs_state.vel.to_numpy()[:, 0]
    np.testing.assert_allclose(qvel, 0, atol=0.1)

    if show_viewer:
        scene.viewer.stop()


@pytest.mark.parametrize("xml_path", ["xml/test_equality.xml"])
@pytest.mark.parametrize("gs_solver", [gs.constraint_solver.CG])
@pytest.mark.parametrize("gs_integrator", [gs.integrator.Euler])
@pytest.mark.parametrize("backend", [gs.cpu], indirect=True)
def test_equality_joint(gs_sim, mj_sim):
    # there is an equality constraint
    np.testing.assert_allclose(gs_sim.rigid_solver.n_equalities, 1, 0.1)

    qpos = np.array((0.0, -1.0))
    qvel = np.array((1, -0.3))
    simulate_and_check_mujoco_consistency(gs_sim, mj_sim, qpos, qvel, num_steps=150)

    # check if the two joints are equal
    gs_qpos = gs_sim.rigid_solver.qpos.to_numpy()[:, 0]
    np.testing.assert_allclose(gs_qpos[0], gs_qpos[1], atol=1e-3)


Review comment (suggestion):
`assert gs_sim.rigid_solver.n_equalities == 1` would do fine in this case, or `assert (gs_sim.rigid_solver.n_equalities == 1).all()` in case it is a vector.

Code after:

    if show_viewer:
        scene.viewer.stop()


def test_nonconvex_collision(show_viewer):
    scene = gs.Scene(show_viewer=show_viewer, show_FPS=False)
    tank = scene.add_entity(
        gs.morphs.Mesh(
            file="meshes/tank.obj",
            scale=5.0,
            fixed=True,
            euler=(90, 0, 0),
        ),
    )
    ball = scene.add_entity(
        gs.morphs.Sphere(
            radius=0.05,
            pos=(0.0, 0.0, 0.8),
        ),
        surface=gs.surfaces.Default(
            color=(0.5, 0.7, 0.9, 1.0),
        ),
    )
    scene.build()

    ball.set_dofs_velocity(np.random.rand(ball.n_dofs) * 0.8)
    for i in range(1500):
        scene.step()

    qvel = scene.sim.rigid_solver.dofs_state.vel.to_numpy()[:, 0]
    np.testing.assert_allclose(qvel, 0, atol=0.1)

    if show_viewer:
        scene.viewer.stop()


@pytest.mark.parametrize("model_name", ["mimic_hinges"])
@pytest.mark.parametrize("gs_solver", [gs.constraint_solver.CG])
@pytest.mark.parametrize("gs_integrator", [gs.integrator.Euler])
@pytest.mark.parametrize("backend", [gs.cpu], indirect=True)
def test_equality_joint(gs_sim, mj_sim):
    # there is an equality constraint
    assert gs_sim.rigid_solver.n_equalities == 1

    qpos = np.array((0.0, -1.0))
    qvel = np.array((1, -0.3))
    simulate_and_check_mujoco_consistency(gs_sim, mj_sim, qpos, qvel, num_steps=300, atol=1e-8)

    # check if the two joints are equal
    gs_qpos = gs_sim.rigid_solver.qpos.to_numpy()[:, 0]
Repository: Genesis-Embodied-AI/Genesis
File: genesis/utils/terrain.py (Python)

Code before:
import os
import math
import numpy as np
import pickle
import trimesh

import genesis as gs
from genesis.ext import fast_simplification
from genesis.ext.isaacgym import terrain_utils as isaacgym_terrain_utils
from genesis.options.morphs import Terrain

from .misc import get_assets_dir


def parse_terrain(morph: Terrain, surface):
    """
    Generate mesh (and height field) according to configurations passed by morph.

    ------------------------------------------------------------------------------------------------------
    If morph.height_field is not passed, generate the height field by
        n_subterrains    : Tuple[int, int]     = (3, 3)     # number of subterrains in x and y directions
        subterrain_types : Any                 = [
                ['flat_terrain', 'random_uniform_terrain', 'pyramid_sloped_terrain'],
                ['pyramid_sloped_terrain', 'discrete_obstacles_terrain', 'wave_terrain'],
                ['random_uniform_terrain', 'pyramid_stairs_terrain', 'pyramid_sloped_terrain'],
        ]                                                   # types of subterrains in x and y directions
    If morph.height_field is passed, (n_subterrains, subterrain_size, subterrain_types) will be ignored.
    ------------------------------------------------------------------------------------------------------

    Returns
    --------------------------

Review comment (suggestion):
Keep system deps seperated from common deps:
```
import os
import math
import pickle

import numpy as np
import trimesh
```

Code after:
import os
import math
import pickle

import numpy as np
import trimesh

import genesis as gs
from genesis.ext import fast_simplification
from genesis.ext.isaacgym import terrain_utils as isaacgym_terrain_utils
from genesis.options.morphs import Terrain

from .misc import get_assets_dir


def parse_terrain(morph: Terrain, surface):
    """
    Generate mesh (and height field) according to configurations passed by morph.

    ------------------------------------------------------------------------------------------------------
    If morph.height_field is not passed, generate the height field by
        n_subterrains    : Tuple[int, int]     = (3, 3)     # number of subterrains in x and y directions
        subterrain_types : Any                 = [
                ['flat_terrain', 'random_uniform_terrain', 'pyramid_sloped_terrain'],
                ['pyramid_sloped_terrain', 'discrete_obstacles_terrain', 'wave_terrain'],
                ['random_uniform_terrain', 'pyramid_stairs_terrain', 'pyramid_sloped_terrain'],
        ]                                                   # types of subterrains in x and y directions
    If morph.height_field is passed, (n_subterrains, subterrain_size, subterrain_types) will be ignored.
    ------------------------------------------------------------------------------------------------------

    Returns
Repository: Genesis-Embodied-AI/Genesis
File: tests/conftest.py (Python)

Code before:
            ls_tolerance=mj_sim.model.opt.ls_tolerance,
        ),
        show_viewer=show_viewer,
        show_FPS=False,
    )
    gs_robot = scene.add_entity(
        gs.morphs.MJCF(file=xml_path),
        visualize_contact=True,
    )
    gs_sim = scene.sim

    # Force matching Mujoco safety factor for constraint time constant.
    # Note that this time constant affects the penetration depth at rest.
    gs_sim.rigid_solver._sol_constraint_min_resolve_time = 2.0 * gs_sim._substep_dt

    # Joint damping is not properly supported in Genesis for now
    if not dof_damping:
        for joint in gs_robot.joints:
            joint.dofs_damping[:] = 0.0

    scene.build()

    return gs_sim


@pytest.fixture
def box_obj_path(tmp_path):
    """Fixture that generates a temporary cube .obj file"""
    cx, cy, cz = (0.0, 0.0, 0.0)  # Center of the cube
    edge_length = 1.0

    h = edge_length / 2.0

    verts = [
        (cx - h, cy - h, cz - h),  # v0
        (cx + h, cy - h, cz - h),  # v1
        (cx + h, cy + h, cz - h),  # v2
        (cx - h, cy + h, cz - h),  # v3
        (cx - h, cy - h, cz + h),  # v4
        (cx + h, cy - h, cz + h),  # v5
        (cx + h, cy + h, cz + h),  # v6
        (cx - h, cy + h, cz + h),  # v7
    ]

    faces = [
        (1, 2, 3, 4),
        (5, 6, 7, 8),
        (1, 2, 6, 5),
        (2, 3, 7, 6),
        (3, 4, 8, 7),
        (4, 1, 5, 8),

Review comment (suggestion):
I think the scope should be "session" as you expect the same output for all tests? Once this is done, you do not need `uuid` anymore, just call it as you want, e.g. `cube.obj`. It will be created only once anyway so it is perfectly fine.

Code after:
        ),
        show_viewer=show_viewer,
        show_FPS=False,
    )
    gs_robot = scene.add_entity(
        gs.morphs.MJCF(file=xml_path),
        visualize_contact=True,
    )
    gs_sim = scene.sim

    # Force matching Mujoco safety factor for constraint time constant.
    # Note that this time constant affects the penetration depth at rest.
    gs_sim.rigid_solver._sol_constraint_min_resolve_time = 2.0 * gs_sim._substep_dt

    # Joint damping is not properly supported in Genesis for now
    if not dof_damping:
        for joint in gs_robot.joints:
            joint.dofs_damping[:] = 0.0

    scene.build()

    return gs_sim


@pytest.fixture(scope="session")
def cube_verts_and_faces():
    cx, cy, cz = (0.0, 0.0, 0.0)
    edge_length = 1.0

    h = edge_length / 2.0

    verts = [
        (cx - h, cy - h, cz - h),  # v0
        (cx + h, cy - h, cz - h),  # v1
        (cx + h, cy + h, cz - h),  # v2
        (cx - h, cy + h, cz - h),  # v3
        (cx - h, cy - h, cz + h),  # v4
        (cx + h, cy - h, cz + h),  # v5
        (cx + h, cy + h, cz + h),  # v6
        (cx - h, cy + h, cz + h),  # v7
    ]

    faces = [
        (1, 2, 3, 4),
        (5, 6, 7, 8),
        (1, 2, 6, 5),
        (2, 3, 7, 6),
        (3, 4, 8, 7),
        (4, 1, 5, 8),
    ]
    return verts, faces
Repository: Genesis-Embodied-AI/Genesis
File: examples/ddp_multi_gpu.py (Python)

Code before:
import genesis as gs


class TinyMLP(nn.Module):
    def __init__(self, obs_dim: int, act_dim: int) -> None:
        super().__init__()
        self.net = nn.Sequential(
            nn.Linear(obs_dim, 128),
            nn.ReLU(),
            nn.Linear(128, act_dim),
        )

    def forward(self, x):
        return self.net(x.float())


def run_worker(args: argparse.Namespace) -> None:
    # setup
    local_rank = int(os.environ.get("LOCAL_RANK", 0))
    print("local_rank", local_rank)

    os.environ["CUDA_VISIBLE_DEVICES"] = str(local_rank)
    os.environ["TI_VISIBLE_DEVICE"] = str(local_rank)

    gs.init(backend=gs.gpu)

    seed = local_rank
    random.seed(seed)
    np.random.seed(seed)
    torch.manual_seed(seed)
    torch.cuda.manual_seed_all(seed)

    # sim
    scene = gs.Scene(
        viewer_options=gs.options.ViewerOptions(
            camera_pos=(3.5, 0.0, 2.5),
            camera_lookat=(0.0, 0.0, 0.5),
            camera_fov=40,
        ),
        show_viewer=False,
        show_FPS=False,
    )
    scene.add_entity(gs.morphs.Plane())
    scene.add_entity(
        gs.morphs.MJCF(file="xml/franka_emika_panda/panda.xml"),
        visualize_contact=True,
    )
    scene.build(n_envs=args.n_envs)

    # model
    torch.cuda.set_device(0)

Review comment (suggestion):
`os.environ["EGL_DEVICE_ID"] = str(local_rank)` for isolated rendering. Good practice even if not used in practice.

Code after:

import os, argparse, random, numpy as np
import torch, torch.nn as nn, torch.distributed as dist
from torch.nn.parallel import DistributedDataParallel as DDP
import genesis as gs


class TinyMLP(nn.Module):
    def __init__(self, obs_dim: int, act_dim: int) -> None:
        super().__init__()
        self.net = nn.Sequential(
            nn.Linear(obs_dim, 128),
            nn.ReLU(),
            nn.Linear(128, act_dim),
        )

    def forward(self, x):
        return self.net(x.float())


def run_worker(args: argparse.Namespace) -> None:
    # setup
    local_rank = int(os.environ.get("LOCAL_RANK", 0))

    os.environ["CUDA_VISIBLE_DEVICES"] = str(local_rank)
    os.environ["TI_VISIBLE_DEVICE"] = str(local_rank)
    # FIXME: Forcing rendering device is not working reliably on all machines
    # os.environ["EGL_DEVICE_ID"] = str(local_rank)
    gs.init(backend=gs.gpu, seed=local_rank)

    # sim
    scene = gs.Scene(
        viewer_options=gs.options.ViewerOptions(
            camera_pos=(3.5, 0.0, 2.5),
            camera_lookat=(0.0, 0.0, 0.5),
            camera_fov=40,
        ),
        show_viewer=False,
        show_FPS=False,
    )
    scene.add_entity(gs.morphs.Plane())
    scene.add_entity(
        gs.morphs.MJCF(file="xml/franka_emika_panda/panda.xml"),
        visualize_contact=True,
    )
    scene.build(n_envs=args.n_envs)

    # model
    gpu_id = 0
    torch.cuda.set_device(gpu_id)
    dist.init_process_group(backend="nccl", init_method="env://")
Repository: Genesis-Embodied-AI/Genesis
File: genesis/utils/mjcf.py (Python)

Code before:
import os
import xml.etree.ElementTree as ET
import contextlib
from pathlib import Path
from itertools import chain
from bisect import bisect_right
import io

import numpy as np
import trimesh
from trimesh.visual.texture import TextureVisuals
from PIL import Image

if os.name != "nt":  # POSIX → keep current behaviour
    from wurlitzer import pipes
else:  # Windows → provide a dummy replacement

    @contextlib.contextmanager
    def pipes(stdout=True, stderr=True):  # signature matches wurlitzer.pipes
        yield io.StringIO(), io.StringIO()


import z3
import mujoco
import genesis as gs
from genesis.ext import urdfpy

from . import geom as gu
from . import urdf as uu
from .misc import get_assets_dir, redirect_libc_stderr


MIN_TIMECONST = np.finfo(np.double).eps


def build_model(xml, discard_visual, default_armature=None, merge_fixed_links=False, links_to_keep=()):
    if isinstance(xml, (str, Path)):
        # Make sure that it is pointing to a valid XML content (either file path or string)
        path = os.path.join(get_assets_dir(), xml)

Review comment (suggestion):
You should have a cleaner uniform way of redirecting libc stdout / stderr. I suggest to move all the logic here along with what I already implemented for macOS at least, and use the same cross platform decorator everywhere

Code after:
import os
import xml.etree.ElementTree as ET
import contextlib
from pathlib import Path
from itertools import chain
from bisect import bisect_right
import io

import numpy as np
import trimesh
from trimesh.visual.texture import TextureVisuals
from PIL import Image

import z3
import mujoco
import genesis as gs
from genesis.ext import urdfpy

from . import geom as gu
from . import urdf as uu
from .misc import get_assets_dir, redirect_libc_stderr


MIN_TIMECONST = np.finfo(np.double).eps


def build_model(xml, discard_visual, default_armature=None, merge_fixed_links=False, links_to_keep=()):
    if isinstance(xml, (str, Path)):
        # Make sure that it is pointing to a valid XML content (either file path or string)
        path = os.path.join(get_assets_dir(), xml)
        is_valid_path = False
        try:
            if os.path.exists(path):
                xml = ET.parse(path)
                is_valid_path = True
            else:
                xml = ET.fromstring(xml)
        except ET.ParseError:
            gs.raise_exception_from(f"'{xml}' is not a valid XML file path or string.")
Repository: Genesis-Embodied-AI/Genesis
File: genesis/ext/pyrender/interaction/vec3.py (Python)

Code before:
import numpy as np
from numpy.typing import NDArray

class Vec3:
    v: NDArray[np.float32]

    def __init__(self, v: NDArray[np.float32]):
        assert v.shape == (3,), f"Vec3 must be initialized with a 3-element array, got {v.shape}"
        self.v = v

    def __add__(self, other: 'Vec3') -> 'Vec3': return Vec3(self.v + other.v)
    def __sub__(self, other: 'Vec3') -> 'Vec3': return Vec3(self.v - other.v)
    def __mul__(self, other: float) -> 'Vec3': return Vec3(self.v * other)
    def __rmul__(self, other: float) -> 'Vec3': return Vec3(self.v * other)
    def dot(self, other: 'Vec3') -> float: return np.dot(self.v, other.v)
    def cross(self, other: 'Vec3') -> 'Vec3': return Vec3(np.cross(self.v, other.v))

    def normalized(self) -> 'Vec3': return Vec3(self.v / (np.linalg.norm(self.v) + 1e-24))

    def copy(self) -> 'Vec3': return Vec3(self.v.copy())

    def __repr__(self) -> str: return f"Vec3({self.v[0]}, {self.v[1]}, {self.v[2]})"

    @classmethod
    def from_xyz(cls, x: float, y: float, z: float) -> 'Vec3': return cls(np.array([x, y, z], dtype=np.float32))

    @classmethod
    def zero(cls): return cls(np.array([0, 0, 0]))
    @classmethod
    def one(cls): return cls(np.array([1, 1, 1]))

Review comment (performance):
I'm not convinced by the relevant of this class. It just add one extra layer of indirection that is hindering performance will not really making code much clearer. This kind of approach is rarely used in Python.

Code after:
import numpy as np
from numpy.typing import NDArray


class Vec3:
    """
    Use this wrapper around np.array if you want to ensure adherence to float32 arithmethic
    with runtime checks, and avoid hidden and costly conversions between float32 and float64.

    This also makes vector dimensionality explicit for linting and static analysis.
    """
    v: NDArray[np.float32]

    def __init__(self, v: NDArray[np.float32]):
        assert v.shape == (3,), f"Vec3 must be initialized with a 3-element array, got {v.shape}"
        assert v.dtype == np.float32, f"Vec3 must be initialized with a float32 array, got {v.dtype}"
        self.v = v

    def __add__(self, other: 'Vec3') -> 'Vec3':
        return Vec3(self.v + other.v)

    def __sub__(self, other: 'Vec3') -> 'Vec3':
        return Vec3(self.v - other.v)

    def __mul__(self, other: float) -> 'Vec3':
        return Vec3(self.v * np.float32(other))

    def __rmul__(self, other: float) -> 'Vec3':
        return Vec3(self.v * np.float32(other))
Repository: Genesis-Embodied-AI/Genesis
File: genesis/ext/pyrender/interaction/ray.py (Python)

Code before:
from dataclasses import dataclass
from genesis.ext.pyrender.interaction.vec3 import Vec3


EPSILON = 1e-6


class Ray:
    origin: Vec3
    direction: Vec3

    def __init__(self, origin: Vec3, direction: Vec3):
        self.origin = origin
        self.direction = direction.normalized()

    def __repr__(self) -> str:
        return f"Ray(origin={self.origin}, direction={self.direction})"


@dataclass
class RayHit:
    is_hit: bool
    distance: float
    normal: Vec3
    position: Vec3
    object_idx: int


Review comment (suggestion):
Skip one line and relative import:
```python
from dataclasses import dataclass

from .vec3 import Vec3
```

Code after:
from dataclasses import dataclass

from .vec3 import Vec3


EPSILON = 1e-6


class Ray:
    origin: Vec3
    direction: Vec3

    def __init__(self, origin: Vec3, direction: Vec3):
        self.origin = origin
        self.direction = direction.normalized()

    def __repr__(self) -> str:
        return f"Ray(origin={self.origin}, direction={self.direction})"


@dataclass
class RayHit:
    is_hit: bool
    distance: float
    normal: Vec3
    position: Vec3
    object_idx: int