-
Notifications
You must be signed in to change notification settings - Fork 15
Fix opw solver #229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix opw solver #229
Changes from all commits
b524b10
706a9e9
a3f9404
74ac20b
885bcdf
95e5661
1432d93
0afe751
0b60b89
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -72,6 +72,13 @@ class SolverCfg: | |||||
| when multiple solutions are available. | ||||||
| """ | ||||||
|
|
||||||
| user_qpos_limits: List[float] | None = None | ||||||
| """ | ||||||
| User defined Joint position limits [2, DOF] for the solver. | ||||||
| If not provided (None), this value will replace by joint limits defined in urdf when solver init from robot. | ||||||
| If provided, the solver will use the intersection of user defined limits and urdf limits as the final joint limits. | ||||||
|
Comment on lines
+75
to
+79
|
||||||
| """ | ||||||
|
|
||||||
| @abstractmethod | ||||||
| def init_solver(self, device: torch.device, **kwargs) -> "BaseSolver": | ||||||
| pass | ||||||
|
|
@@ -165,6 +172,8 @@ def __init__(self, cfg: SolverCfg = None, device: str = None, **kwargs): | |||||
| device=self.device, | ||||||
| ) | ||||||
|
|
||||||
| self._init_qpos_limits() | ||||||
|
|
||||||
| def set_ik_nearest_weight( | ||||||
| self, ik_weight: np.ndarray, joint_ids: np.ndarray | None = None | ||||||
| ) -> bool: | ||||||
|
|
@@ -223,51 +232,106 @@ def get_ik_nearest_weight(self): | |||||
| """ | ||||||
| return self.ik_nearest_weight | ||||||
|
|
||||||
| def set_position_limits( | ||||||
| def _init_qpos_limits(self): | ||||||
| self.lower_qpos_limits = None | ||||||
| self.upper_qpos_limits = None | ||||||
| if self.cfg.user_qpos_limits is not None: | ||||||
| # robot qpos limits from config, expected shape [DOF, 2] | ||||||
| user_qpos_limits = torch.tensor( | ||||||
| self.cfg.user_qpos_limits, dtype=torch.float32, device=self.device | ||||||
| ) | ||||||
| if user_qpos_limits.shape == (2, self.dof): | ||||||
| self.set_qpos_limits( | ||||||
| lower_qpos_limits=user_qpos_limits[0], | ||||||
| upper_qpos_limits=user_qpos_limits[1], | ||||||
| ) | ||||||
| elif user_qpos_limits.shape == (self.dof, 2): | ||||||
| self.set_qpos_limits( | ||||||
| lower_qpos_limits=user_qpos_limits[:, 0], | ||||||
| upper_qpos_limits=user_qpos_limits[:, 1], | ||||||
| ) | ||||||
| else: | ||||||
| logger.log_error( | ||||||
|
||||||
| logger.log_error( | |
| raise ValueError( |
Copilot
AI
Apr 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update_with_robot_limit() clamps lower_qpos_limits and upper_qpos_limits independently, but doesn’t re-check that the resulting limits still satisfy lower <= upper. If user limits are inconsistent with robot limits, this can produce an impossible interval that later IK code can’t satisfy. Add a final validation step (and error/fallback) after clamping.
Copilot
AI
Apr 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set_qpos_limits() doesn’t validate that the provided limit arrays have length self.dof. If a caller passes a mismatched length, the solver can end up with incorrect limit tensors and later indexing/clamping bugs. Add an explicit length/shape check (and fail fast) before storing the tensors.
Copilot
AI
Apr 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set_qpos_limits() no longer validates that the provided limits match the solver DOF. If the lengths differ, zip() will silently truncate and later indexing (e.g., in OPW kernels) can go out-of-bounds or use wrong limits. Add a length/shape check against self.dof (and return False / raise) before storing the limits.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -29,7 +29,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| OPWparam, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| opw_fk_kernel, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| opw_ik_kernel, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| opw_best_ik_kernel, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| opw_ik_select_kernel, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| wp_vec6f, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from embodichain.utils.device_utils import standardize_device_string | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -72,6 +72,9 @@ class OPWSolverCfg(SolverCfg): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Parameters for the inverse-kinematics method. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ik_params: dict | None = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # safe margin for joint limits, in radians | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| safe_margin: float = 5.0 * np.pi / 180.0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def init_solver( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self, device: torch.device = torch.device("cpu"), **kwargs | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> "OPWSolver": | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -247,50 +250,87 @@ def get_ik_warp( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| N_SOL = 8 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| DOF = 6 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| n_sample = target_xpos.shape[0] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| kernel_device = standardize_device_string(self.device) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if target_xpos.shape == (4, 4): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| target_xpos_batch = target_xpos[None, :, :] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| target_xpos_batch = target_xpos[None, :, :].to(kernel_device) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| target_xpos_batch = target_xpos | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| target_xpos_batch = target_xpos.to(kernel_device) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
250
to
+258
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| target_xpos_wp = wp.from_torch(target_xpos_batch.reshape(-1)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| all_qpos_wp = wp.zeros( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| n_sample * N_SOL * DOF, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dtype=float, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| device=standardize_device_string(self.device), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| device=standardize_device_string(kernel_device), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| all_ik_valid_wp = wp.zeros( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| n_sample * N_SOL, dtype=int, device=standardize_device_string(self.device) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| n_sample * N_SOL, dtype=int, device=standardize_device_string(kernel_device) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # TODO: whether require gradient | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| offsets_ = self.offsets.to(standardize_device_string(kernel_device)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sign_corrections_ = self.sign_corrections.to( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
270
to
+272
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| standardize_device_string(kernel_device) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| lower_limits_ = wp_vec6f( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.lower_qpos_limits[0], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.lower_qpos_limits[1], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.lower_qpos_limits[2], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.lower_qpos_limits[3], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.lower_qpos_limits[4], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.lower_qpos_limits[5], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| upper_limits_ = wp_vec6f( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.upper_qpos_limits[0], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.upper_qpos_limits[1], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.upper_qpos_limits[2], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.upper_qpos_limits[3], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.upper_qpos_limits[4], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.upper_qpos_limits[5], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+275
to
+289
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| lower_limits_ = wp_vec6f( | |
| self.lower_qpos_limits[0], | |
| self.lower_qpos_limits[1], | |
| self.lower_qpos_limits[2], | |
| self.lower_qpos_limits[3], | |
| self.lower_qpos_limits[4], | |
| self.lower_qpos_limits[5], | |
| ) | |
| upper_limits_ = wp_vec6f( | |
| self.upper_qpos_limits[0], | |
| self.upper_qpos_limits[1], | |
| self.upper_qpos_limits[2], | |
| self.upper_qpos_limits[3], | |
| self.upper_qpos_limits[4], | |
| self.upper_qpos_limits[5], | |
| if self.lower_qpos_limits is None or self.upper_qpos_limits is None: | |
| raise ValueError( | |
| "Joint position limits must be initialized before calling the OPW IK solver." | |
| ) | |
| lower_limits_ = wp_vec6f( | |
| float(self.lower_qpos_limits[0].item() if isinstance(self.lower_qpos_limits[0], torch.Tensor) else self.lower_qpos_limits[0]), | |
| float(self.lower_qpos_limits[1].item() if isinstance(self.lower_qpos_limits[1], torch.Tensor) else self.lower_qpos_limits[1]), | |
| float(self.lower_qpos_limits[2].item() if isinstance(self.lower_qpos_limits[2], torch.Tensor) else self.lower_qpos_limits[2]), | |
| float(self.lower_qpos_limits[3].item() if isinstance(self.lower_qpos_limits[3], torch.Tensor) else self.lower_qpos_limits[3]), | |
| float(self.lower_qpos_limits[4].item() if isinstance(self.lower_qpos_limits[4], torch.Tensor) else self.lower_qpos_limits[4]), | |
| float(self.lower_qpos_limits[5].item() if isinstance(self.lower_qpos_limits[5], torch.Tensor) else self.lower_qpos_limits[5]), | |
| ) | |
| upper_limits_ = wp_vec6f( | |
| float(self.upper_qpos_limits[0].item() if isinstance(self.upper_qpos_limits[0], torch.Tensor) else self.upper_qpos_limits[0]), | |
| float(self.upper_qpos_limits[1].item() if isinstance(self.upper_qpos_limits[1], torch.Tensor) else self.upper_qpos_limits[1]), | |
| float(self.upper_qpos_limits[2].item() if isinstance(self.upper_qpos_limits[2], torch.Tensor) else self.upper_qpos_limits[2]), | |
| float(self.upper_qpos_limits[3].item() if isinstance(self.upper_qpos_limits[3], torch.Tensor) else self.upper_qpos_limits[3]), | |
| float(self.upper_qpos_limits[4].item() if isinstance(self.upper_qpos_limits[4], torch.Tensor) else self.upper_qpos_limits[4]), | |
| float(self.upper_qpos_limits[5].item() if isinstance(self.upper_qpos_limits[5], torch.Tensor) else self.upper_qpos_limits[5]), |
Copilot
AI
Apr 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If qpos_seed has an unexpected shape, this logs an error but still proceeds to build qpos_seed_wp from qpos_seed_, which will be undefined and raise at runtime. After logging, return/raise (or fall back to a zeros seed) to avoid an UnboundLocalError.
| logger.log_error( | |
| f"Invalid shape for qpos_seed: {qpos_seed.shape}. Expected ({n_sample}, {DOF}) or ({DOF},)." | |
| ) | |
| error_msg = ( | |
| f"Invalid shape for qpos_seed: {qpos_seed.shape}. " | |
| f"Expected ({n_sample}, {DOF}) or ({DOF},)." | |
| ) | |
| logger.log_error(error_msg) | |
| raise ValueError(error_msg) |
Copilot
AI
Apr 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default joint_weight uses torch.ones(..., dtype=float), but dtype must be a torch.dtype (e.g., torch.float32). As written, this will raise a TypeError when joint_weight is not provided. Also consider extracting values via .item() (and/or ensuring the tensor is on CPU) before constructing wp_vec6f to avoid issues with 0-dim CUDA tensors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self._datadoes not appear to be defined onRobot(this class usesself.body_dataelsewhere). This will raiseAttributeErrorduring solver initialization. Useself.body_data.qpos_limits(orself.get_qpos_limits(...)) to fetch joint limits instead.