Fix tendon ID resolver dtype handling#5485
Fix tendon ID resolver dtype handling#5485AntoineRichard wants to merge 3 commits intoisaac-sim:developfrom
Conversation
Convert torch int64 fixed and spatial tendon ID tensors to Warp int32 arrays in the PhysX and Newton articulation resolvers. This keeps tendon indexing consistent with the existing env, joint, and body ID resolver behavior.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR fixes a dtype mismatch bug in the tendon ID resolver methods for both PhysX and Newton articulation backends. When users pass torch.int64 tensors (the default dtype from torch.arange()), the resolvers now correctly convert them to int32 before creating Warp arrays, mirroring the existing behavior of _resolve_env_ids, _resolve_joint_ids, and _resolve_body_ids. The fix is straightforward and includes appropriate regression tests.
Architecture Impact
Self-contained. The changes are internal to the _resolve_fixed_tendon_ids and _resolve_spatial_tendon_ids helper methods in both backends. These methods are called by public tendon property setters (e.g., set_fixed_tendon_stiffness, set_spatial_tendon_stiffness), so any code passing torch.int64 indices to those APIs will now work correctly instead of causing Warp dtype errors.
Implementation Verdict
Ship it — Minor style suggestion below, but not blocking.
Test Coverage
Good. The PR adds two targeted regression tests (test_resolve_fixed_tendon_ids_converts_int64_tensor and test_resolve_spatial_tendon_ids_converts_int64_tensor) that directly verify the fix for both PhysX and Newton backends. The tests:
- Create articulations with tendons
- Pass
torch.int64tensors (the problematic case) - Assert the output is a
wp.arraywithint32dtype
One gap: the tests only run on "cpu" device. Consider adding "cuda:0" to catch any device-specific issues, though this is minor since the dtype conversion logic is device-agnostic.
CI Status
No CI results available yet — recommend waiting for CI to pass before merge.
Findings
🔵 Improvement: isaaclab_physx/.../articulation.py:4521-4527 and isaaclab_newton/.../articulation.py:3822-3828 — Tensor handling could use .contiguous() for safety
When a tensor slice is passed (as tested: tendon_ids[:2]), the slice may not be contiguous in memory. wp.from_torch generally handles this, but for robustness and consistency with other resolvers in the codebase that call .contiguous(), consider:
if isinstance(tendon_ids, torch.Tensor):
if tendon_ids.dtype == torch.int64:
tendon_ids = tendon_ids.to(torch.int32)
return wp.from_torch(tendon_ids.contiguous(), dtype=wp.int32)This is defensive programming — Warp may already handle non-contiguous tensors, but explicit is better than implicit.
🔵 Improvement: isaaclab_physx/.../articulation.py:4518-4529 and isaaclab_newton/.../articulation.py:3819-3830 — Code duplication across backends
The _resolve_fixed_tendon_ids and _resolve_spatial_tendon_ids implementations are now nearly identical between PhysX and Newton backends (and to _resolve_env_ids, _resolve_joint_ids, _resolve_body_ids). Consider extracting this pattern to a shared utility in the base class or a helper module to reduce maintenance burden. However, this is out of scope for a bug fix PR.
🔵 Improvement: test_articulation_iface.py:565,577 — Tests only cover CPU device
The new tests use device="cpu". While the fix is device-agnostic, testing on "cuda:0" (when available) would increase confidence. Consider:
@_index_resolution_backends
@pytest.mark.parametrize("device", ["cpu", "cuda:0"])
def test_resolve_fixed_tendon_ids_converts_int64_tensor(self, backend, device):
...🔵 Improvement: Changelog fragments could be more specific
The changelog entries state "accept torch.int64 tendon ID tensors" but could mention that this aligns with the existing behavior of env/joint/body ID resolvers, providing more context for users reading release notes.
Overall: Clean, correct fix that addresses a real usability issue. The implementation correctly mirrors the established pattern for other ID resolvers. Ready to merge after CI passes.
Greptile SummaryThis PR fixes a dtype mismatch bug in Confidence Score: 5/5This PR is safe to merge — it is a focused, correct bug fix that mirrors the established resolver pattern. The fix is minimal, consistent across both backends, and correctly implements the same int64→int32 conversion pattern already used by all other ID resolvers. Tests are added to verify the fix. No regressions to existing behaviour since prior inputs (list, None, slice, wp.array) are handled identically. No files require special attention Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[tendon_ids input] --> B{isinstance list?}
B -- yes --> C[wp.array int32]
B -- no --> D{is None?}
D -- yes --> E[return ALL_TENDON_INDICES]
D -- no --> F{isinstance torch.Tensor?}
F -- yes --> G{dtype == int64?}
G -- yes --> H[.to int32]
H --> I[wp.from_torch int32]
G -- no --> I
F -- no --> J{== slice None?}
J -- yes --> E
J -- no --> K[return as-is wp.array or other]
Reviews (1): Last reviewed commit: "Fix tendon ID resolver dtype handling" | Re-trigger Greptile |
Mark the isaaclab test-only change as skipped for the fragment gate so it does not produce a package changelog entry or version bump.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
No new commits since the previous review — the HEAD is still at 7bd220c5. The previous review findings remain applicable.
Implementation Verdict
Ship it — The fix is correct and well-tested. Previous suggestions (.contiguous() for safety, multi-device testing, code deduplication) were style/defensive improvements, not blocking issues.
CI Status
Most checks are now green (pre-commit ✅, Build Wheel ✅, changelog check ✅). Docker builds and installation tests are still pending. Recommend merging once Installation Tests passes.
Description
This PR fixes tendon ID resolver dtype handling in the PhysX and Newton articulation backends.
The fixed and spatial tendon ID resolvers now mirror the existing env, joint, and body ID resolver behavior by converting user-provided
torch.int64tensors to Warpint32arrays before they are passed to indexed Warp paths.Related to #5329.
Type of change
Screenshots
N/A.
Testing
./isaaclab.sh -f./isaaclab.sh -p tools/changelog/cli.py check develop./isaaclab.sh -p tools/changelog/cli.py compile --package isaaclab_physx --dry-run./isaaclab.sh -p tools/changelog/cli.py compile --package isaaclab_newton --dry-run./isaaclab.sh -p -m pytest source/isaaclab/test/assets/test_articulation_iface.py::TestArticulationIndexResolution(blocked locally:ModuleNotFoundError: No module named 'torch'from/usr/bin/python3.12)Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<package>/changelog.d/; versions are compiled by nightly CICONTRIBUTORS.mdor my name already exists there