Skip to content

Update pytorch kinematic solver benchmark param#240

Merged
matafela merged 2 commits into
mainfrom
cj/update-pytorch-kinematics-benchmark
Apr 20, 2026
Merged

Update pytorch kinematic solver benchmark param#240
matafela merged 2 commits into
mainfrom
cj/update-pytorch-kinematics-benchmark

Conversation

@matafela
Copy link
Copy Markdown
Collaborator

Description

Update pytorch kinematic solver benchmark joint limits.

Type of change

  • Enhancement (non-breaking change which improves an existing functionality)

Screenshots

Checklist

  • I have run the black . command to format the code base.
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • Dependencies have been updated, if applicable.

Copilot AI review requested due to automatic review settings April 20, 2026 08:15
@matafela matafela requested a review from yuecideng April 20, 2026 08:16
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the PyTorch kinematic solver benchmark configuration by changing the joint-limit ranges used for sampling and for initializing the solver.

Changes:

  • Replaces the prior wide joint limits for the PyTorch benchmark with a narrow fixed range (1.0–2.5 rad for all joints), leaving the previous values commented out.
  • Adds TODO/commented debug lines around investigating failed IK samples.
Comments suppressed due to low confidence (1)

scripts/benchmark/robotics/kinematic_solver/run_benchmark.py:435

  • The commented-out failure debugging block (including an ipdb import) should be removed from the benchmark script. If this is useful, consider gating it behind a --debug flag and using structured logging rather than leaving commented debug code in the main path.
        cpu_t_err, cpu_r_err = get_pose_err(fk_xpos_cpu, check_xpos_cpu)

        cpu_result = {
            "cost_time_ms": cpu_elapsed * 1000.0,

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

PYTORCH_LOWER_LIMITS = [-6.2832, -6.2832, -3.1416, -6.2832, -6.2832, -6.2832]
PYTORCH_UPPER_LIMITS = [6.2832, 6.2832, 3.1416, 6.2832, 6.2832, 6.2832]

# TODO: Easy to failed if use full joint range, consider adding a margin to avoid sampling near the joint limits.
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

Spelling/grammar in the TODO: "Easy to failed" should be corrected (e.g., "Easy to fail").

Suggested change
# TODO: Easy to failed if use full joint range, consider adding a margin to avoid sampling near the joint limits.
# TODO: Easy to fail if using the full joint range; consider adding a margin to avoid sampling near the joint limits.

Copilot uses AI. Check for mistakes.
PYTORCH_LOWER_LIMITS = [-6.2832, -6.2832, -3.1416, -6.2832, -6.2832, -6.2832]
PYTORCH_UPPER_LIMITS = [6.2832, 6.2832, 3.1416, 6.2832, 6.2832, 6.2832]

# TODO: Easy to failed if use full joint range, consider adding a margin to avoid sampling near the joint limits.
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

This TODO says to "consider adding a margin", but the benchmark already samples with a margin via _sample_qpos(...) (and passes margin=1e-1). As written it’s misleading—either remove it or update it to describe the actual failure mode and why the existing margin isn’t sufficient.

Suggested change
# TODO: Easy to failed if use full joint range, consider adding a margin to avoid sampling near the joint limits.
# NOTE: The benchmark already samples qpos with a nonzero margin in
# `_sample_qpos(...)`. Even with that margin, benchmarking the Pytorch solver
# over its full nominal joint range has proven unreliable in practice, so we
# currently use a narrower range here to keep the benchmark stable.

Copilot uses AI. Check for mistakes.
Comment on lines +43 to +47
# TODO: Easy to failed if use full joint range, consider adding a margin to avoid sampling near the joint limits.
# PYTORCH_LOWER_LIMITS = [-6.2832, -6.2832, -3.1416, -6.2832, -6.2832, -6.2832]
# PYTORCH_UPPER_LIMITS = [6.2832, 6.2832, 3.1416, 6.2832, 6.2832, 6.2832]
PYTORCH_LOWER_LIMITS = [1.0, 1.0, 1.0, 1.0, 1.0, 1.0]
PYTORCH_UPPER_LIMITS = [2.5, 2.5, 2.5, 2.5, 2.5, 2.5]
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

The new PYTORCH_*_LIMITS values (1.0–2.5 rad for every joint) both constrain sampling and are passed into PytorchSolverCfg.user_qpos_limits, changing the solver’s own joint-limit constraints. This likely makes the benchmark unrepresentative (and can artificially inflate success rate) compared to using the URDF limits with a sampling margin. Consider restoring the URDF/robot limits and only narrowing the sampling range (or compute a margin from the solver’s configured limits).

Suggested change
# TODO: Easy to failed if use full joint range, consider adding a margin to avoid sampling near the joint limits.
# PYTORCH_LOWER_LIMITS = [-6.2832, -6.2832, -3.1416, -6.2832, -6.2832, -6.2832]
# PYTORCH_UPPER_LIMITS = [6.2832, 6.2832, 3.1416, 6.2832, 6.2832, 6.2832]
PYTORCH_LOWER_LIMITS = [1.0, 1.0, 1.0, 1.0, 1.0, 1.0]
PYTORCH_UPPER_LIMITS = [2.5, 2.5, 2.5, 2.5, 2.5, 2.5]
# Keep the solver's configured joint limits aligned with the robot/URDF limits.
# If benchmark sampling needs to avoid edge cases near joint boundaries, apply a
# separate sampling margin at the sampling site rather than narrowing these
# limits, because these values may also be passed into solver configuration.
PYTORCH_LOWER_LIMITS = [-6.2832, -6.2832, -3.1416, -6.2832, -6.2832, -6.2832]
PYTORCH_UPPER_LIMITS = [6.2832, 6.2832, 3.1416, 6.2832, 6.2832, 6.2832]

Copilot uses AI. Check for mistakes.
Comment on lines +46 to +47
PYTORCH_LOWER_LIMITS = [1.0, 1.0, 1.0, 1.0, 1.0, 1.0]
PYTORCH_UPPER_LIMITS = [2.5, 2.5, 2.5, 2.5, 2.5, 2.5]
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

PR description checklist indicates documentation and tests were updated/added, but this PR (as shown) only changes the benchmark script. If docs/tests were intended, they appear to be missing from the diff; otherwise, please update the description/checklist to match the actual changes.

Copilot uses AI. Check for mistakes.
@matafela matafela merged commit e0e16ae into main Apr 20, 2026
3 of 4 checks passed
@matafela matafela deleted the cj/update-pytorch-kinematics-benchmark branch April 20, 2026 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants