Skip to content

Conversation

@michaellin6
Copy link
Contributor

Description

Fixes typo in NullSpacePostureTask documentation. Adds additional tests for NullSpacePostureTask. Makes QP solver for Pink a parameter in config file so users can choose other solver options, but still defaults to 'daqp'.

Fixes # (issue)
Fixes issues raised by community user: #4417

Type of change

  • New feature (non-breaking change which adds functionality)
  • Documentation update

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

…a tests for NullSpacePosture Task. Making Pink QP solver a parameter in config.
@github-actions github-actions bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Feb 3, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 3, 2026

Greptile Overview

Greptile Summary

This PR addresses issue #4417 by fixing documentation and adding configurability to the Pink IK controller. The changes include:

  • Documentation Fix (Partial): Corrected the mathematical formula in the null space posture task documentation from (q* - q) to (q - q*) at lines 32 and 75
  • QP Solver Configuration: Added qp_solver parameter to PinkIKControllerCfg allowing users to choose between different solvers (defaults to "daqp")
  • Enhanced Testing: Added two comprehensive integration tests (test_solve_ik_with_null_space_posture_task and test_solve_ik_with_different_posture_offsets) that verify the IK solver behavior with the null space posture task

Critical Issue Found:
The documentation change creates an inconsistency between the updated formulas (lines 32, 75) and both:

  1. The actual implementation at line 200 which uses pin.difference(configuration.model, self.target_q, configuration.q) computing (q* - q)
  2. The unchanged docstring at line 176 which still documents the old formula (q* - q)

This inconsistency needs to be resolved by either reverting the documentation changes or updating the implementation to match.

Confidence Score: 2/5

  • This PR contains a critical documentation-implementation mismatch that needs resolution before merging
  • The QP solver configuration change is solid and well-tested, but the documentation fix introduced an inconsistency between the mathematical formulas in the docs and the actual implementation, which could confuse users and lead to incorrect usage
  • Pay close attention to null_space_posture_task.py - the documentation changes conflict with the implementation

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/controllers/pink_ik/null_space_posture_task.py Updated error formula documentation but introduced inconsistency with implementation
source/isaaclab/isaaclab/controllers/pink_ik/pink_ik.py Made QP solver configurable via cfg parameter, straightforward change
source/isaaclab/isaaclab/controllers/pink_ik/pink_ik_cfg.py Added qp_solver configuration parameter with clear documentation
source/isaaclab/test/controllers/test_null_space_posture_task.py Added comprehensive integration tests for solve_ik and improved error message formatting

Sequence Diagram

sequenceDiagram
    participant User
    participant PinkIKController
    participant Config as PinkIKControllerCfg
    participant Solver as Pink solve_ik
    participant Task as NullSpacePostureTask

    User->>Config: Set qp_solver (e.g., "daqp", "quadprog")
    User->>PinkIKController: Initialize with config
    PinkIKController->>Task: Set up NullSpacePostureTask
    Task->>Task: Initialize with controlled_frames & controlled_joints
    
    User->>PinkIKController: compute(curr_joint_pos, dt)
    PinkIKController->>Task: compute_error(configuration)
    Task->>Task: pin.difference(target_q, current_q)
    Note over Task: Computes (q* - q) [Implementation]<br/>Docs say (q - q*) [Inconsistency]
    Task-->>PinkIKController: Error vector
    
    PinkIKController->>Task: compute_jacobian(configuration)
    Task->>Task: Build null space projector N = I - J+ @ J
    Task-->>PinkIKController: Null space Jacobian
    
    PinkIKController->>Solver: solve_ik(config, tasks, dt, solver=cfg.qp_solver)
    Note over Solver: Uses configurable solver<br/>(previously hardcoded "daqp")
    Solver-->>PinkIKController: Joint velocities
    PinkIKController-->>User: Target joint positions
Loading

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.

4 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

.. math::

\mathbf{e}(\mathbf{q}) = \mathbf{M} \cdot (\mathbf{q}^* - \mathbf{q})
\mathbf{e}(\mathbf{q}) = \mathbf{M} \cdot (\mathbf{q} - \mathbf{q}^*)
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation now states the formula is (q - q*) but the actual implementation at line 200 uses pin.difference(configuration.model, self.target_q, configuration.q) which computes (q* - q). Either the documentation fix is incorrect, or the implementation needs to be updated to match.

Same issue at line 75 and in the docstring at line 176.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 3, 2026

Additional Comments (1)

source/isaaclab/isaaclab/controllers/pink_ik/null_space_posture_task.py
Docstring still uses old formula (q* - q) but lines 32 and 75 were changed to (q - q*). Need to ensure all three locations are consistent with the actual implementation at line 200.

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