-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fixes in place modification to body_pose mdp term when body_ids is either slice or int
#4233
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
Conversation
Greptile SummaryThis PR fixes an in-place modification bug in Key Changes:
Issue Found: Other Issues:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User Config
participant Manager as ManagerBase
participant Cfg as SceneEntityCfg
participant Obs as body_pose_w()
participant Tensor as PyTorch Tensor
User->>Manager: Configure body_ids (int, list[int], or slice)
Manager->>Cfg: resolve(scene)
alt body_ids is int
Cfg->>Cfg: Convert int to list[int]
Note over Cfg: Lines 250-251 in scene_entity_cfg.py
end
Note over Cfg: After resolve:<br/>body_ids is either list[int] or slice
Manager->>Obs: Call observation function
Obs->>Tensor: Index with body_ids
alt body_ids is slice
Tensor-->>Obs: Returns VIEW (shares memory)
Note over Obs: ⚠️ Needs .clone() to avoid in-place modification
else body_ids is list[int]
Tensor-->>Obs: Returns COPY (separate memory)
Note over Obs: ✓ No .clone() needed
end
Obs->>Obs: Subtract env_origins
Obs->>Manager: Return pose
|
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.
Additional Comments (2)
-
source/isaaclab/isaaclab/envs/mdp/observations.py, line 157-158 (link)logic: the
isinstance(asset_cfg.body_ids, int)check will never be True at runtime.SceneEntityCfg._resolve_body_names()convertsinttolist[int]before observations run (seescene_entity_cfg.py:250-251). Also, PyTorch's advanced indexing withlist[int]already creates a copy, so onlysliceneeds.clone(). -
source/isaaclab/docs/CHANGELOG.rst, line 10 (link)syntax: missing backtick after
:meth:
3 files reviewed, 2 comments
body_pose mdp term when body_ids is either slice or int
|
Thanks for the fix and addressing the comments! :) |
…either slice or int (isaac-sim#4233) # Description This PR fixes inplace operation issue reported in isaac-sim#4211 in observation.py, that observation mdp term body_pose when body_ids is either slice or int Fixes isaac-sim#4211 ## Type of change - Bug fix (non-breaking change which fixes an issue) ## Checklist - [x] I have read and understood the [contribution guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html) - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there --------- Co-authored-by: Kelly Guo <kellyg@nvidia.com>
Description
This PR fixes inplace operation issue reported in #4211 in observation.py, that observation mdp term body_pose when body_ids is either slice or int
Fixes #4211
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there