Conversation
Add right_arm, left_arm, right_leg, left_leg properties to RobotModel base class with isinstance type checking and NotImplementedError handling. Also add backward-compatible aliases to individual robot models: - PR2: right_arm, left_arm, right_arm_with_torso, left_arm_with_torso - Panda, Fetch, Kuka: arm, arm_with_torso (for single-arm robots) - Nextage, R8_6: right_arm, left_arm This allows using full names (robot.right_arm) while maintaining backward compatibility with abbreviated names (robot.rarm).
Test right_arm, left_arm, arm aliases and their end_coords variants for dual-arm robots (PR2) and single-arm robots (Fetch, Kuka). Also verify that non-existent limbs (legs) return None.
- batch_ik_demo.py: Use arm/right_arm instead of rarm - pybullet_robot_interface.py: Use arm/arm_end_coords instead of rarm
There was a problem hiding this comment.
Pull request overview
This PR adds full naming convention aliases for robot limbs to improve code readability and intuitiveness. The main changes introduce properties like right_arm, left_arm, and arm as aliases for the abbreviated forms rarm, larm, and rarm (for single-arm robots), while maintaining full backward compatibility with the original naming.
- Adds base class properties in
RobotModelthat provide default alias behavior for all robot models - Implements specific aliases in individual robot model classes to override base behavior where needed
- Updates examples and notebooks to demonstrate the new naming convention
- Fixes a typo in PR2's
gripper_distance()error message
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
skrobot/model/robot_model.py |
Adds base class properties for limb and end_coords aliases with fallback behavior |
skrobot/models/pr2.py |
Adds right_arm, left_arm aliases and updates gripper_distance() to accept new names; fixes error message typo |
skrobot/models/fetch.py |
Adds arm and arm_with_torso aliases for single-arm robot |
skrobot/models/panda.py |
Adds arm alias for single-arm robot |
skrobot/models/kuka.py |
Adds arm alias for single-arm robot |
skrobot/models/nextage.py |
Adds right_arm and left_arm aliases for dual-arm robot |
skrobot/models/r8_6.py |
Adds right_arm, left_arm, and torso variants for dual-arm robot |
tests/skrobot_tests/model_tests/test_robot_model.py |
Adds comprehensive tests for new naming convention aliases |
examples/pybullet_robot_interface.py |
Updates to use arm and arm_end_coords for Kuka robot |
examples/pr2_inverse_kinematics.py |
Updates to use right_arm_end_coords throughout |
examples/batch_ik_demo.py |
Updates to use new full naming convention for all robots |
examples/grasp_and_pull_box.py |
Updates gripper calls and variable names to use new convention |
examples/collision_free_trajectory.py |
Updates variable names from rarm_* to right_arm_* |
examples/notebooks/jupyter_collision_free_with_base.ipynb |
Updates notebook to use right_arm_end_coords and arm_point_history |
examples/notebooks/colab_jupyter_viewer_demo.ipynb |
Updates notebook to use right_arm_end_coords and new gripper naming |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
skrobot/model/robot_model.py
Outdated
| # New naming convention properties (forward compatibility) | ||
| # These provide access using full names (right_arm, left_arm, etc.) | ||
| # while maintaining backward compatibility with abbreviated names. |
There was a problem hiding this comment.
The comment says "forward compatibility" but this is actually about providing more intuitive naming while maintaining backward compatibility with the abbreviated names. Consider updating the comment to clarify that these properties provide improved naming conventions (full names like "right_arm") while the abbreviated names ("rarm") remain available for backward compatibility.
| # New naming convention properties (forward compatibility) | |
| # These provide access using full names (right_arm, left_arm, etc.) | |
| # while maintaining backward compatibility with abbreviated names. | |
| # New naming convention properties with more intuitive full names. | |
| # These provide access using full names (right_arm, left_arm, etc.) | |
| # while keeping the abbreviated names (rarm, larm, etc.) available for | |
| # backward compatibility. |
skrobot/models/pr2.py
Outdated
| right_arm = rarm | ||
| left_arm = larm | ||
| right_arm_with_torso = rarm_with_torso | ||
| left_arm_with_torso = larm_with_torso | ||
|
|
There was a problem hiding this comment.
Direct assignment of cached_property descriptors may not work as intended. When assigning right_arm = rarm where rarm is decorated with @cached_property, both attributes share the same descriptor object. However, the caching behavior of cached_property typically relies on the attribute name for storing cached values in the instance dictionary. This approach may lead to unexpected behavior or multiple cached instances.
Consider using explicit property wrappers instead:
@cached_property
def right_arm(self):
return self.rarmThis ensures proper caching semantics and makes the aliasing relationship explicit.
| right_arm = rarm | |
| left_arm = larm | |
| right_arm_with_torso = rarm_with_torso | |
| left_arm_with_torso = larm_with_torso | |
| @cached_property | |
| def right_arm(self): | |
| return self.rarm | |
| @cached_property | |
| def left_arm(self): | |
| return self.larm | |
| @cached_property | |
| def right_arm_with_torso(self): | |
| return self.rarm_with_torso | |
| @cached_property | |
| def left_arm_with_torso(self): | |
| return self.larm_with_torso |
skrobot/models/fetch.py
Outdated
| arm = rarm | ||
| arm_with_torso = rarm_with_torso | ||
|
|
||
| @property |
There was a problem hiding this comment.
Direct assignment of cached_property descriptors may not work as intended. When assigning arm = rarm and arm_with_torso = rarm_with_torso where these are decorated with @cached_property, both attributes share the same descriptor object. This may lead to unexpected caching behavior.
Consider using explicit property wrappers instead for clarity and to ensure proper caching semantics.
| arm = rarm | |
| arm_with_torso = rarm_with_torso | |
| @property | |
| @property | |
| def arm(self): | |
| return self.rarm | |
| @property | |
| def arm_with_torso(self): | |
| return self.rarm_with_torso | |
| @property |
skrobot/models/panda.py
Outdated
| return model | ||
|
|
||
| # New naming convention aliases (backward compatible) | ||
| arm = rarm |
There was a problem hiding this comment.
Direct assignment of cached_property descriptor may not work as intended. When assigning arm = rarm where rarm is decorated with @cached_property, both attributes share the same descriptor object, which may lead to unexpected caching behavior.
Consider using an explicit property wrapper instead for clarity and to ensure proper caching semantics.
| arm = rarm | |
| @property | |
| def arm(self): | |
| return self.rarm |
skrobot/models/kuka.py
Outdated
| return self.angle_vector(av) | ||
|
|
||
| # New naming convention aliases (backward compatible) | ||
| arm = rarm |
There was a problem hiding this comment.
Direct assignment of cached_property descriptor may not work as intended. When assigning arm = rarm where rarm is decorated with @cached_property, both attributes share the same descriptor object, which may lead to unexpected caching behavior.
Consider using an explicit property wrapper instead for clarity and to ensure proper caching semantics.
| arm = rarm | |
| @property | |
| def arm(self): | |
| return self.rarm |
skrobot/models/nextage.py
Outdated
| right_arm = rarm | ||
| left_arm = larm | ||
|
|
||
| @property |
There was a problem hiding this comment.
Direct assignment of cached_property descriptors may not work as intended. When assigning right_arm = rarm and left_arm = larm where these are decorated with @cached_property, both attributes share the same descriptor object, which may lead to unexpected caching behavior.
Consider using explicit property wrappers instead for clarity and to ensure proper caching semantics.
| right_arm = rarm | |
| left_arm = larm | |
| @property | |
| @property | |
| def right_arm(self): | |
| return self.rarm | |
| @property | |
| def left_arm(self): | |
| return self.larm | |
| @property |
| limb = getattr(self, attr_name, None) | ||
| if isinstance(limb, RobotModel): | ||
| return limb | ||
| except NotImplementedError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| coords = getattr(self, attr_name, None) | ||
| if isinstance(coords, CascadedCoords): | ||
| return coords | ||
| except NotImplementedError: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
No description provided.