From f27f5f84b0a94cf80c0881d2a3682039aa60814e Mon Sep 17 00:00:00 2001 From: Steven Palma Date: Tue, 20 May 2025 13:05:41 +0200 Subject: [PATCH] Apply suggestions from code review 1 Co-authored-by: Simon Alibert <75076266+aliberts@users.noreply.github.com> --- .gitattributes | 2 +- lerobot/common/cameras/__init__.py | 2 +- lerobot/common/cameras/camera.py | 4 ++-- lerobot/common/cameras/opencv/configuration_opencv.py | 1 - lerobot/common/utils/control_utils.py | 1 - lerobot/find_cameras.py | 1 - tests/cameras/test_opencv.py | 5 +++-- tests/cameras/test_realsense.py | 10 +++------- 8 files changed, 10 insertions(+), 16 deletions(-) diff --git a/.gitattributes b/.gitattributes index 096d1613..7d89f37b 100644 --- a/.gitattributes +++ b/.gitattributes @@ -18,4 +18,4 @@ *.arrow filter=lfs diff=lfs merge=lfs -text *.json !text !filter !merge !diff tests/artifacts/cameras/*.png filter=lfs diff=lfs merge=lfs -text -tests/artifacts/cameras/*.bag filter=lfs diff=lfs merge=lfs -text +*.bag filter=lfs diff=lfs merge=lfs -text diff --git a/lerobot/common/cameras/__init__.py b/lerobot/common/cameras/__init__.py index 272c4e9e..917bb789 100644 --- a/lerobot/common/cameras/__init__.py +++ b/lerobot/common/cameras/__init__.py @@ -13,5 +13,5 @@ # limitations under the License. from .camera import Camera -from .configs import CameraConfig +from .configs import CameraConfig, ColorMode from .utils import make_cameras_from_configs diff --git a/lerobot/common/cameras/camera.py b/lerobot/common/cameras/camera.py index 8d88794c..47233571 100644 --- a/lerobot/common/cameras/camera.py +++ b/lerobot/common/cameras/camera.py @@ -33,7 +33,7 @@ class Camera(abc.ABC): pass @abc.abstractmethod - def connect(self, do_warmup_read: bool = True) -> None: + def connect(self, warmup: bool = True) -> None: pass @abc.abstractmethod @@ -41,7 +41,7 @@ class Camera(abc.ABC): pass @abc.abstractmethod - def async_read(self, timeout_ms: float = 2000) -> np.ndarray: + def async_read(self, timeout_ms: float = ...) -> np.ndarray: pass @abc.abstractmethod diff --git a/lerobot/common/cameras/opencv/configuration_opencv.py b/lerobot/common/cameras/opencv/configuration_opencv.py index 5d7c4195..8cced062 100644 --- a/lerobot/common/cameras/opencv/configuration_opencv.py +++ b/lerobot/common/cameras/opencv/configuration_opencv.py @@ -53,7 +53,6 @@ class OpenCVCameraConfig(CameraConfig): index_or_path: int | Path color_mode: ColorMode = ColorMode.RGB - channels: int = 3 # NOTE(Steven): Why is this a config? rotation: Cv2Rotation = Cv2Rotation.NO_ROTATION def __post_init__(self): diff --git a/lerobot/common/utils/control_utils.py b/lerobot/common/utils/control_utils.py index 730144f3..3ac792a5 100644 --- a/lerobot/common/utils/control_utils.py +++ b/lerobot/common/utils/control_utils.py @@ -38,7 +38,6 @@ from lerobot.common.utils.robot_utils import busy_wait from lerobot.common.utils.utils import get_safe_torch_device, has_method -# NOTE(Steven): Consider integrating this in camera class def log_control_info(robot: Robot, dt_s, episode_index=None, frame_index=None, fps=None): log_items = [] if episode_index is not None: diff --git a/lerobot/find_cameras.py b/lerobot/find_cameras.py index 6227eaba..f24c0d14 100644 --- a/lerobot/find_cameras.py +++ b/lerobot/find_cameras.py @@ -32,7 +32,6 @@ from lerobot.common.cameras.opencv.camera_opencv import OpenCVCamera from lerobot.common.cameras.opencv.configuration_opencv import OpenCVCameraConfig logger = logging.getLogger(__name__) -# logging.basicConfig(level=logging.INFO, format="%(asctime)s - %(levelname)s - %(module)s - %(message)s") def find_all_opencv_cameras() -> List[Dict[str, Any]]: diff --git a/tests/cameras/test_opencv.py b/tests/cameras/test_opencv.py index 44cb9bb8..f17cb013 100644 --- a/tests/cameras/test_opencv.py +++ b/tests/cameras/test_opencv.py @@ -39,7 +39,8 @@ TEST_IMAGE_PATHS = [ ] -def test_base_class_implementation(): +def test_abc_implementation(): + """Instantiation should raise an error if the class doesn't implement abstract methods/properties.""" config = OpenCVCameraConfig(index_or_path=0) _ = OpenCVCamera(config) @@ -168,7 +169,7 @@ def test_async_read_before_connect(): Cv2Rotation.ROTATE_270, ], ) -def test_all_rotations(rotation, index_or_path): +def test_rotation(rotation, index_or_path): filename = os.path.basename(index_or_path) dimensions = filename.split("_")[-1].split(".")[0] # Assumes filenames format (_wxh.png) original_width, original_height = map(int, dimensions.split("x")) diff --git a/tests/cameras/test_realsense.py b/tests/cameras/test_realsense.py index fb5e5009..bb6ad667 100644 --- a/tests/cameras/test_realsense.py +++ b/tests/cameras/test_realsense.py @@ -39,13 +39,8 @@ BAG_FILE_PATH = os.path.join(TEST_ARTIFACTS_DIR, "test_rs.bag") # NOTE(Steven): Missing tests for depth # NOTE(Steven): Takes 20sec, the patch being the biggest bottleneck # NOTE(Steven): more tests + assertions? -if not os.path.exists(BAG_FILE_PATH): - print(f"Warning: Bag file not found at {BAG_FILE_PATH}. Some tests might fail or be skipped.") - def mock_rs_config_enable_device_from_file(rs_config_instance, sn): - if not os.path.exists(BAG_FILE_PATH): - raise FileNotFoundError(f"Test bag file not found: {BAG_FILE_PATH}") return rs_config_instance.enable_device_from_file(BAG_FILE_PATH, repeat_playback=True) @@ -53,7 +48,8 @@ def mock_rs_config_enable_device_bad_file(rs_config_instance, sn): return rs_config_instance.enable_device_from_file("non_existent_file.bag", repeat_playback=True) -def test_base_class_implementation(): +def test_abc_implementation(): + """Instantiation should raise an error if the class doesn't implement abstract methods/properties.""" config = RealSenseCameraConfig(serial_number=42) _ = RealSenseCamera(config) @@ -181,7 +177,7 @@ def test_async_read_before_connect(): ], ) @patch("pyrealsense2.config.enable_device", side_effect=mock_rs_config_enable_device_from_file) -def test_all_rotations(mock_enable_device, rotation): +def test_rotation(mock_enable_device, rotation): config = RealSenseCameraConfig(serial_number=42, rotation=rotation) camera = RealSenseCamera(config) camera.connect(do_warmup_read=False)