Skip to content

Conversation

@ooctipus
Copy link
Collaborator

@ooctipus ooctipus commented Oct 28, 2025

Description

This PR fixes the issue reported in #3753 where the advanced indexing failed due to shape mismatch.
This PR is an alternative solution to #3754,

The fixes is tricky, the most elegant and performant way I found so far is separate code path when env_id is None vs env_id is tensor. if we don't do so, applying left hand side with self._prev_applied_actions[env_ids, :] where env_ids is 2d will cause the lfs shape to be ill formed.

Fixes #3753

Type of change

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

Screenshots

Please attach before and after screenshots of the change if applicable.

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
  • 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 and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@github-actions github-actions bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Oct 28, 2025
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Greptile Summary

This PR fixes a PyTorch advanced indexing bug in EMAJointPositionToLimitsAction.reset() that caused shape mismatch errors when resetting environments with a subset of joints enabled. The issue occurred because advanced indexing with 2D env_ids tensors produced incompatible shapes during assignment operations. The fix introduces conditional code paths: when env_ids is None, it uses efficient slice indexing, and when env_ids is a tensor, it uses explicit 2D indexing with env_ids[:, None] combined with .view(len(env_ids), -1) to ensure proper shape compatibility. This action class is part of the MDP (Markov Decision Process) action space framework in IsaacLab, where it maps normalized actions to joint position limits with exponential moving average smoothing for robot control tasks.

Important Files Changed

Filename Score Overview
source/isaaclab/isaaclab/envs/mdp/actions/joint_actions_to_limits.py 4/5 Fixed advanced indexing shape mismatch by separating reset logic for None vs tensor env_ids cases with proper reshaping
source/isaaclab/docs/CHANGELOG.rst 5/5 Added changelog entry documenting the bug fix for version 0.47.3
source/isaaclab/config/extension.toml 5/5 Bumped patch version from 0.47.2 to 0.47.3 to reflect the bug fix

Confidence score: 4/5

  • This PR addresses a specific bug with a targeted fix that should resolve the shape mismatch issue without introducing side effects
  • Score reflects that while the fix is well-reasoned and addresses the root cause, there are no accompanying tests to validate the fix works correctly for all edge cases (acknowledged by unchecked test checkbox in PR description)
  • Pay close attention to source/isaaclab/isaaclab/envs/mdp/actions/joint_actions_to_limits.py - ensure the conditional logic correctly handles both code paths and that the .view(len(env_ids), -1) reshape produces expected dimensions for various joint subset configurations

Sequence Diagram

sequenceDiagram
    participant User
    participant Environment
    participant EMAJointPositionToLimitsAction
    participant AssetData
    
    User->>Environment: reset(env_ids)
    Environment->>EMAJointPositionToLimitsAction: reset(env_ids)
    
    alt env_ids is None
        EMAJointPositionToLimitsAction->>EMAJointPositionToLimitsAction: super().reset(slice(None))
        EMAJointPositionToLimitsAction->>AssetData: Get joint_pos[:, joint_ids]
        AssetData-->>EMAJointPositionToLimitsAction: Return joint positions
        EMAJointPositionToLimitsAction->>EMAJointPositionToLimitsAction: Set _prev_applied_actions[:]
    else env_ids is tensor
        EMAJointPositionToLimitsAction->>EMAJointPositionToLimitsAction: super().reset(env_ids)
        EMAJointPositionToLimitsAction->>AssetData: Get joint_pos[env_ids[:, None], joint_ids]
        Note over EMAJointPositionToLimitsAction: Advanced indexing with 2D env_ids causes shape mismatch
        AssetData-->>EMAJointPositionToLimitsAction: Return joint positions (wrong shape)
        EMAJointPositionToLimitsAction->>EMAJointPositionToLimitsAction: view(len(env_ids), -1)
        EMAJointPositionToLimitsAction->>EMAJointPositionToLimitsAction: Set _prev_applied_actions[env_ids, :]
        Note over EMAJointPositionToLimitsAction: Left-hand side shape is ill-formed due to 2D env_ids
    end
    
    EMAJointPositionToLimitsAction-->>Environment: Reset complete
    Environment-->>User: Environment ready
Loading

3 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

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.

[Bug Report] Subset-joint reset fails in EMAJointPositionToLimitsAction

1 participant