-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Fixes advanced indexing shape mismatch when resetting prev_action isaaclab.envs.mdp.actions.JointPositionToLimitsAction #3865
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
base: main
Are you sure you want to change the base?
Conversation
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.
Greptile Overview
Greptile Summary
This PR fixes a PyTorch advanced indexing bug in EMAJointPositionToLimitsAction.reset() that caused shape mismatch errors when resetting environments with a subset of joints enabled. The issue occurred because advanced indexing with 2D env_ids tensors produced incompatible shapes during assignment operations. The fix introduces conditional code paths: when env_ids is None, it uses efficient slice indexing, and when env_ids is a tensor, it uses explicit 2D indexing with env_ids[:, None] combined with .view(len(env_ids), -1) to ensure proper shape compatibility. This action class is part of the MDP (Markov Decision Process) action space framework in IsaacLab, where it maps normalized actions to joint position limits with exponential moving average smoothing for robot control tasks.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/envs/mdp/actions/joint_actions_to_limits.py | 4/5 | Fixed advanced indexing shape mismatch by separating reset logic for None vs tensor env_ids cases with proper reshaping |
| source/isaaclab/docs/CHANGELOG.rst | 5/5 | Added changelog entry documenting the bug fix for version 0.47.3 |
| source/isaaclab/config/extension.toml | 5/5 | Bumped patch version from 0.47.2 to 0.47.3 to reflect the bug fix |
Confidence score: 4/5
- This PR addresses a specific bug with a targeted fix that should resolve the shape mismatch issue without introducing side effects
- Score reflects that while the fix is well-reasoned and addresses the root cause, there are no accompanying tests to validate the fix works correctly for all edge cases (acknowledged by unchecked test checkbox in PR description)
- Pay close attention to
source/isaaclab/isaaclab/envs/mdp/actions/joint_actions_to_limits.py- ensure the conditional logic correctly handles both code paths and that the.view(len(env_ids), -1)reshape produces expected dimensions for various joint subset configurations
Sequence Diagram
sequenceDiagram
participant User
participant Environment
participant EMAJointPositionToLimitsAction
participant AssetData
User->>Environment: reset(env_ids)
Environment->>EMAJointPositionToLimitsAction: reset(env_ids)
alt env_ids is None
EMAJointPositionToLimitsAction->>EMAJointPositionToLimitsAction: super().reset(slice(None))
EMAJointPositionToLimitsAction->>AssetData: Get joint_pos[:, joint_ids]
AssetData-->>EMAJointPositionToLimitsAction: Return joint positions
EMAJointPositionToLimitsAction->>EMAJointPositionToLimitsAction: Set _prev_applied_actions[:]
else env_ids is tensor
EMAJointPositionToLimitsAction->>EMAJointPositionToLimitsAction: super().reset(env_ids)
EMAJointPositionToLimitsAction->>AssetData: Get joint_pos[env_ids[:, None], joint_ids]
Note over EMAJointPositionToLimitsAction: Advanced indexing with 2D env_ids causes shape mismatch
AssetData-->>EMAJointPositionToLimitsAction: Return joint positions (wrong shape)
EMAJointPositionToLimitsAction->>EMAJointPositionToLimitsAction: view(len(env_ids), -1)
EMAJointPositionToLimitsAction->>EMAJointPositionToLimitsAction: Set _prev_applied_actions[env_ids, :]
Note over EMAJointPositionToLimitsAction: Left-hand side shape is ill-formed due to 2D env_ids
end
EMAJointPositionToLimitsAction-->>Environment: Reset complete
Environment-->>User: Environment ready
3 files reviewed, no comments
jtigue-bdai
left a comment
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.
Looks good.
Description
This PR fixes the issue reported in #3753 where the advanced indexing failed due to shape mismatch.
This PR is an alternative solution to #3754,
The fixes is tricky, the most elegant and performant way I found so far is separate code path when env_id is None vs env_id is tensor. if we don't do so, applying left hand side with self._prev_applied_actions[env_ids, :] where env_ids is 2d will cause the lfs shape to be ill formed.
Fixes #3753
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there