Add cpu num to SimulationCfg & Add some interfaces to robot #51
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances simulation configuration and robot control interfaces by adding CPU thread control and improving user-friendliness for controlling specific robot parts.
- Added
cpu_numparameter toSimulationManagerCfgwith default value of 1 to control CPU thread usage - Added control part name interfaces to
Robotclass for getter/setter methods (qpos, qvel, qf) and limit getters - Updated import path for
SimResourcesfromembodichain.data.assetstoembodichain.data.dataset
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 15 comments.
| File | Description |
|---|---|
| embodichain/lab/sim/sim_manager.py | Adds cpu_num configuration field and applies it to world config; updates import path for SimResources |
| embodichain/lab/sim/objects/robot.py | Adds new methods to get/set joint states and limits for specific control parts by name |
| embodichain/lab/sim/objects/articulation.py | Improves documentation for the get_qpos method with return type details |
| tests/sim/objects/test_robot.py | Adds test case for new control part getter/setter functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if name not in self.control_parts: | ||
| logger.log_error( | ||
| f"The control part '{name}' does not exist in the robot's control parts {self.control_parts}." | ||
| ) | ||
| return self._control_groups[name].link_names |
There was a problem hiding this comment.
The error handling pattern using logger.log_error() doesn't prevent execution from continuing. After logging the error, the code will attempt to access self._control_groups[name] which will raise a KeyError if the name doesn't exist. Consider either raising an exception or returning early after logging the error.
| if not self.control_parts or name not in self.control_parts: | ||
| logger.log_error( | ||
| f"The control part '{name}' does not exist in the robot's control parts." | ||
| ) | ||
| part_joint_ids = self.get_joint_ids(name=name) | ||
| return qf_limits[local_env_ids][:, part_joint_ids, :] |
There was a problem hiding this comment.
The error handling pattern using logger.log_error() doesn't prevent execution from continuing. After logging the error, the code will proceed to call get_joint_ids() which may fail or return incorrect results. Consider either raising an exception or returning early after logging the error.
embodichain/lab/sim/objects/robot.py
Outdated
| Returns: | ||
| torch.Tensor: Joint velocity limits with shape (N, dof, 2), where N is the number of environments. |
There was a problem hiding this comment.
The indentation for the Returns section is inconsistent. It should have one space after the asterisk, matching the Args section format above.
| if not self.control_parts or name not in self.control_parts: | ||
| logger.log_error( | ||
| f"The control part '{name}' does not exist in the robot's control parts." | ||
| ) | ||
| part_joint_ids = self.get_joint_ids(name=name) | ||
| return qpos_limits[local_env_ids][:, part_joint_ids, :] |
There was a problem hiding this comment.
The error handling pattern using logger.log_error() doesn't prevent execution from continuing. After logging the error, the code will proceed to call get_joint_ids() which may fail or return incorrect results. Consider either raising an exception or returning early after logging the error.
| if not self.control_parts or name not in self.control_parts: | ||
| logger.log_error( | ||
| f"The control part '{name}' does not exist in the robot's control parts." | ||
| ) | ||
| part_joint_ids = self.get_joint_ids(name=name) | ||
| return qpos[:, part_joint_ids] |
There was a problem hiding this comment.
The error handling pattern using logger.log_error() doesn't prevent execution from continuing. After logging the error, the code will proceed to call get_joint_ids() which may fail or return incorrect results. Consider either raising an exception or returning early after logging the error.
| """Get the current positions (qpos) of the articulation. | ||
| Returns: | ||
| torch.Tensor: torch.Tensor: Joint positions with shape (N, dof), where N is the number of environments. |
There was a problem hiding this comment.
The docstring has a duplicate "torch.Tensor:" in the Returns section. The return type should only be stated once.
| torch.Tensor: torch.Tensor: Joint positions with shape (N, dof), where N is the number of environments. | |
| torch.Tensor: Joint positions with shape (N, dof), where N is the number of environments. |
embodichain/lab/sim/objects/robot.py
Outdated
| Returns: | ||
| torch.Tensor: Joint effort limits with shape (N, dof, 2), where N is the number of environments. |
There was a problem hiding this comment.
The indentation for the Returns section is inconsistent. It should have one space after the asterisk, matching the Args section format above.
| if not self.control_parts or name not in self.control_parts: | ||
| logger.log_error( | ||
| f"The control part '{name}' does not exist in the robot's control parts." | ||
| ) | ||
| part_joint_ids = self.get_joint_ids(name=name) | ||
| return qvel[:, part_joint_ids] |
There was a problem hiding this comment.
The error handling pattern using logger.log_error() doesn't prevent execution from continuing. After logging the error, the code will proceed to call get_joint_ids() which may fail or return incorrect results. Consider either raising an exception or returning early after logging the error.
| dummy_qpos = torch.max( | ||
| torch.min(dummy_qpos, left_qpos_limits[:, :, 1]), left_qpos_limits[:, :, 0] | ||
| ) | ||
| self.robot.set_qpos(qpos=dummy_qpos, name="left_arm") |
There was a problem hiding this comment.
The test doesn't verify that the qpos values were actually set correctly. After calling set_qpos, the test should retrieve the qpos again and verify that it matches the values that were set.
embodichain/lab/sim/objects/robot.py
Outdated
| Returns: | ||
| torch.Tensor: Joint position limits with shape (N, dof, 2), where N is the number of environments. |
There was a problem hiding this comment.
The indentation for the Returns section is inconsistent. It should have one space after the asterisk, matching the Args section format above.
Description
cpu_numto SimulationCfg and set default value to 1. This change will reduce the current cpu workload while maintains the similar performance.Robotwith control part name supported. This improve user friendly experience.Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
black .command to format the code base.