Repository navigation
Fix reset_joints_within_limits_range clamping unselected joints - #8616
Conversation
The sampled joint positions and velocities were written only to the joints listed in position_range and velocity_range, but the clamp to those joints' soft limits was applied to the whole joint tensor. Joints without a range were moved to the limits of the listed joints, a subset of more than one joint raised a shape mismatch, and joints listed out of order were clamped with the wrong limits. Clamp the samples to the limits of the sampled joints before writing them instead. Build the joint id tensors as integer tensors so that an empty range dictionary no longer raises an IndexError. Signed-off-by: Patrick-SCH03 <wwoo5241@gmail.com>
[Medium impact] The PR appears safe to merge. SummaryThe PR fixes
No actionable issues were found. Review checks were static; tests were not run. Reviews (1) · Last reviewed commit: "Fix reset_joints_within_limits_range cla..." · Reviewed by Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
All reviewed code categories look good.
- PR description: The title and body match the clamp, index-dtype, changelog, and test changes; reported validation is relevant, and no performance evidence is required for this focused reset-path correction.
- Design and architecture: Architecture looks good.
- Scope and over-engineering: No over-engineering issues.
- API: API changes look good.
- Compatibility and deprecation: No breaking changes identified.
- Implementation: Implementation looks good.
- Style consistency: Style consistency looks good.
- Test quality: Test quality looks good.
No blocking issues. No inline findings.
Automated review; human maintainers own approval decisions.
AntoineRichard
left a comment
There was a problem hiding this comment.
Thanks for the PR @Patrick-SCH03!
|
|
||
| # store the joint pos ids (used later to sample the joint positions) | ||
| self._pos_joint_ids = torch.tensor(pos_joint_ids, device=self._pos_ranges.device) | ||
| self._pos_joint_ids = torch.tensor(pos_joint_ids, dtype=torch.long, device=self._pos_ranges.device) |
|
|
||
| # store the joint vel ids (used later to sample the joint positions) | ||
| self._vel_joint_ids = torch.tensor(vel_joint_ids, device=self._vel_ranges.device) | ||
| self._vel_joint_ids = torch.tensor(vel_joint_ids, dtype=torch.long, device=self._vel_ranges.device) |
There was a problem hiding this comment.
This can be a lot smaller. The one-joint, subset and out-of-order cases all fail because of the same clamp line, and the empty pos and empty vel cases mirror each other. Two cases cover every fixed line: listed keys out of joint order with ranges wider than the soft limits, with an empty dict on the other side, then swapped. Those two fail on every single-line revert of the fix that the current five catch, and they also catch velocity limits applied in the wrong joint order. Inlining the env double removes _make_env. Could you also move it to source/isaaclab/test/envs/mdp/test_events.py, next to test_actions.py and test_terminations.py?
Signed-off-by: Patrick-SCH03 <wwoo5241@gmail.com>
|
Thanks @AntoineRichard! Done in 2fcab12:
I checked the two cases against single-line reverts of the fix: each clamp line, each |
|
@AntoineRichard friendly ping: both requests are addressed in 2fcab12 (int32 joint ids, and the test is down to two cases in |
AntoineRichard
left a comment
There was a problem hiding this comment.
Thanks for the fixes!
|
run-ci |
|
run-ci |
|
run-ci |
Description
reset_joints_within_limits_rangesamples values only for the joints listed inposition_range/velocity_rangeand writes them into those columns, but then clamps the whole(num_envs, num_joints)tensor with the soft limits of the listed joints only:The velocity branch does the same. Depending on which joints are listed, this:
RuntimeError: The size of tensor a (4) must match the size of tensor b (2) at non-singleton dimension 1;{"j2|j3": ..., "j0|j1": ...}): applies each joint's limits to a different joint;position_rangeorvelocity_range({}): raisesIndexError: tensors used as indices must be long, int, byte or bool tensorsin__init__, because the ids are built withtorch.tensor([]), which is a float tensor.This clamps the samples with the limits of the sampled joints before writing them into those columns, and builds the id tensors with
dtype=torch.int32. Listing every joint in joint order gives the same result as before.No task config in the repository uses this term today, so only users of the public API are affected (the subset case currently fails with the
RuntimeErrorabove). I did not find an existing issue for this. Draft #8218 also edits this__call__(moving it toenv_mask) but keeps the whole-tensor clamp, so the two will need a small rebase whichever lands first.Type of change
Release backport
developChecklist
pre-commitchecks withuv run isaaclab --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bump package versions — CI handles that)CONTRIBUTORS.mdor my name already exists thereTesting
test/envs/mdp/test_events.pywith an inline sim-free articulation double and four joints with distinct soft limits. Two cases: two joints listed out of joint order with a range wider than every soft limit inposition_rangeand an emptyvelocity_range, then swapped. The listed joints must reach exactly their own soft limits, and every other joint position and velocity must keep its default. Both cases fail ondevelopand pass with the fix.dtype=torch.int32, or not writing the position or velocity samples each turns one case red.test_root_state_events.pyandtest_terminations.pystill pass (CPU, 6 passed together with the new test).checkandformatand codespell are clean on the changed files, and I checked the changelog fragment name and format against the rules intools/changelog/cli.py. I ran these directly because the fulluv run isaaclab --formatenvironment isn't available on my Windows machine.