-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Fixes in place modification to observation mdp term body_pose 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
base: main
Are you sure you want to change the base?
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
| pose = asset.data.body_pose_w[:, asset_cfg.body_ids, :7] | ||
| if isinstance(asset_cfg.body_ids, (slice, int)): | ||
| pose = pose.clone() # if slice or int, make a copy to avoid modifying original data |
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.
Or we always clone? I don't think it's that expensive?
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.
Can you check if this issue arises some place else by accident
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.
I checked this should be the only place
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