Skip to content

Fix tendon ID resolver dtype handling#5485

Open
AntoineRichard wants to merge 3 commits intoisaac-sim:developfrom
AntoineRichard:antoiner/fix-tendon-id-resolvers
Open

Fix tendon ID resolver dtype handling#5485
AntoineRichard wants to merge 3 commits intoisaac-sim:developfrom
AntoineRichard:antoiner/fix-tendon-id-resolvers

Conversation

@AntoineRichard
Copy link
Copy Markdown
Collaborator

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.int64 tensors to Warp int32 arrays before they are passed to indexed Warp paths.

Related to #5329.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation (not applicable; no docs change required)
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog using fragments under source/<package>/changelog.d/; versions are compiled by nightly CI
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

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.
@github-actions github-actions Bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels May 4, 2026
Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.int64 tensors (the problematic case)
  • Assert the output is a wp.array with int32 dtype

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-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 4, 2026

Greptile Summary

This PR fixes a dtype mismatch bug in _resolve_fixed_tendon_ids and _resolve_spatial_tendon_ids across both the PhysX and Newton articulation backends. When users pass torch.int64 tensors, the methods now convert them to torch.int32 before calling wp.from_torch, mirroring the established pattern already used by the env, joint, and body ID resolvers.

Confidence Score: 5/5

This 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

Filename Overview
source/isaaclab_physx/isaaclab_physx/assets/articulation/articulation.py Adds int64→int32 conversion and wp.from_torch return for torch.Tensor inputs in both _resolve_fixed_tendon_ids and _resolve_spatial_tendon_ids; logic is correct and mirrors existing body/joint resolvers
source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation.py Same fix as physx backend — int64 tensor conversion applied consistently to both tendon resolver methods
source/isaaclab/test/assets/test_articulation_iface.py Adds two new backend-parameterised tests verifying that int64 torch tensors are resolved to int32 wp.arrays for both fixed and spatial tendon resolvers
source/isaaclab_physx/changelog.d/antoiner-fix-tendon-id-resolvers.rst New changelog entry for the physx tendon dtype fix
source/isaaclab_newton/changelog.d/antoiner-fix-tendon-id-resolvers.rst New changelog entry for the newton tendon dtype fix

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]
Loading

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.
Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant