Skip to content

Fix false placement failures in the HandOver benchmark - #676

Merged
Yuan-Xinyi merged 7 commits into
xinyi/bench-skillsfrom
cj/bench-skills
Sep 24, 2026
Merged

Yuan-Xinyi merged 7 commits into
xinyi/bench-skillsfrom
cj/bench-skills

Conversation

@matafela

@matafela matafela commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Description

Fix false placement failures in the HandOver benchmark caused by objects settling below the commanded release height, and align the benchmark with the updated UR10 tutorial.

  • Switch the HandOver tutorial and benchmark from UR5 to UR10.
  • Move the initial object position to XY (-0.2, 0.02) and the delivery target to (-0.2, -0.2, 0.6).
  • Use gripper-specific closing defaults: 0.55 for Robotiq 2F-140 and 0.036 for PGI, while preserving explicit overrides.
  • Replace the previous 3 cm 3D placement threshold with independent limits: XY error ≤ 3 cm and absolute height error ≤ 10 cm. Existing drop detection remains unchanged.
  • Report XY and height errors separately while retaining 3D distance as a diagnostic.
  • Initialize missing replay diagnostics to avoid an unbound variable error.
  • Add regression coverage for settling offsets, tolerance boundaries, invalid measurements, and result reporting.
  • Update benchmark documentation and clarify that historical results have not been recomputed.

Also incorporate upstream changes from main:

Type of change

  • Bug fix
  • Enhancement
  • Documentation update

Validation

  • Atomic-action benchmark tests: 58 passed.
  • black --check --diff ., Python compilation, API documentation coverage, and git diff --check passed.
  • Full simulation benchmarks have not been rerun after the placement-scoring fix.
  • Reviewed affected agent context; no update was needed because the changes preserve the documented architecture.

Screenshots

Not applicable.

Checklist

  • I have run black . to format the codebase; formatting was verified using black --check --diff ..
  • Reviewed affected documentation and agent context.
  • API documentation coverage check passed.
  • Added regression tests for the placement-scoring fix.

matafela and others added 6 commits September 22, 2026 17:59
Co-authored-by: matafela <chenjian@dexforce.com>
Co-authored-by: Jietao Chen <chenjietao@dexforce.top>
Publish render state only for active visual consumers and share publication within explicit render frames. Preserve native-window refresh, coordinate camera/recording/Viser reads, and cover backend stepping with regression tests.
@matafela
matafela requested a review from Yuan-Xinyi September 23, 2026 08:17
@greptile-apps

greptile-apps Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

The PR is not yet safe to merge because the previously reported benchmark-version mismatch remains unresolved.

Fix All in CodexFindings

  1. P1 Benchmark versions remain stale ▶
Fix with agent prompt
### Issue 1
scripts/benchmark/motion_generation/suites/atomic_franka_pgi_curobo.yaml:undefined-96
This changes the microwave asset, interaction targets, and Press/Twist sampling parameters, which changes the benchmark scenario distribution. However, this suite remains `atomic_franka_pgi_curobo_smoke_v3`, and the related pose-batch and randomized suites also retain their old versions. Reports use these versions to identify comparable results, so new results would be mislabeled as compatible with the previous protocols. Increment the versions of all three affected suites.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

This PR updates the HandOver benchmark and tutorial for UR10, introduces gripper-specific closing defaults, and changes placement scoring to distinguish horizontal error from signed settling height. It also incorporates broader upstream work for Franka FEP inverse kinematics, shared visual-state publication, and microwave atomic-action assets and tutorials.

  • Scores settled HandOver placement using XY error ≤ 3 cm and signed height error from −10 cm to +3 cm.
  • Reports XY, signed height, and diagnostic 3D delivery errors separately.
  • Aligns the HandOver tutorial and benchmark on UR10, revised scene coordinates, and gripper-specific defaults.
  • Adds focused scoring and gripper-command regression coverage.
  • Adds Franka FEP inverse kinematics and consolidates render-state publication for active visual consumers.
  • Documents that historical HandOver results were not recomputed under the new scenario and scoring protocol.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart LR
    Scene[UR10 HandOver scene] --> Execute[Execute grasp and transfer]
    Execute --> Release[Release and settle]
    Release --> Measure[Measure final object pose]
    Measure --> XY[XY error ≤ 3 cm]
    Measure --> Z[Signed Z error: −10 cm to +3 cm]
    Measure --> Drop[Existing drop detection]
    XY --> Placed[Placed result]
    Z --> Placed
    Drop --> Placed
    Measure --> Diagnostic[Retain 3D distance as diagnostic]
Loading

Reviews (2) · Last reviewed commit: "fix"

articulations:
- id: microwave
asset_path: MicrowaveOven/microwave_oven_with_inertials.urdf
asset_path: Microwave/microwave.urdf

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Benchmark versions remain stale

This changes the microwave asset, interaction targets, and Press/Twist sampling parameters, which changes the benchmark scenario distribution. However, this suite remains atomic_franka_pgi_curobo_smoke_v3, and the related pose-batch and randomized suites also retain their old versions. Reports use these versions to identify comparable results, so new results would be mislabeled as compatible with the previous protocols. Increment the versions of all three affected suites.

Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/benchmark/motion_generation/suites/atomic_franka_pgi_curobo.yaml
Line: 96

Comment:
**Benchmark versions remain stale**

This changes the microwave asset, interaction targets, and Press/Twist sampling parameters, which changes the benchmark scenario distribution. However, this suite remains `atomic_franka_pgi_curobo_smoke_v3`, and the related pose-batch and randomized suites also retain their old versions. Reports use these versions to identify comparable results, so new results would be mislabeled as compatible with the previous protocols. Increment the versions of all three affected suites.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Codex Fix in Claude Code

Comment thread scripts/tutorials/atomic_action/tutorial_utils.py Outdated
@matafela matafela changed the title Cj/bench skills Fix false placement failures in the HandOver benchmark Sep 23, 2026
@Yuan-Xinyi

Copy link
Copy Markdown
Collaborator

Reviewed the layer delta only (de8cab0..e50c14e, 6 files); the rest of the file list is a clean merge of main with no conflict resolutions. One blocker.

1. This breaks an existing test. hand_over.py:119 adds parser.set_defaults(robot="ur10"), but tests/sim/atomic_actions/test_tutorial_utils.py:1009 asserts every atomic-action tutorial defaults to ur5. That assertion is identical on the base, this head, and main, and the PR does not touch it:

FAILED tests/sim/atomic_actions/test_tutorial_utils.py::test_all_atomic_action_tutorials_accept_both_robot_choices[hand_over]
AssertionError: assert 'ur10' == 'ur5'

The base commit de8cab0 has no set_defaults, so this is introduced here. "Atomic-action benchmark tests: 58 passed" is exactly tests/benchmark/atomic_action and does not reach this test; widening to tests/sim/atomic_actions tests/benchmark gives 1 failed / 1045 passed. Make the expected default per-module.

2. tutorial_utils.py:803 docstring says 0.7, the constant is 0.55. DEFAULT_ROBOTIQ_GRIPPER_CLOSE_QPOS = 0.55, and the PR body says 0.55 too.

3. The height budget is two-sided, its rationale is one-sided. _delivery_goal_reached uses abs(final_z - 0.60) <= 0.10, so Z in [0.50, 0.70] passes. The lower half is the settling case you are fixing. The upper half lets the object finish 10 cm above the commanded pose and still score placed, which gravity does not explain. Nothing else in the ladder checks the receiving hand: hand_is_released is called only for left_hand (line 321). I could not construct a reproducible case since the trajectory does open the right hand, so this is a precision point rather than a bug, but an asymmetric bound would match the documented reasoning: allow 0.10 below the commanded Z, keep the horizontal budget above it.

4. The report caveat names one change out of three. BENCHMARK_REPORT.md:9-12 mentions only the tolerance change. This PR also switches HandOver from UR5 to UR10 and moves both the object start and the delivery target 20 cm in X. The hand_over section at line 113 and its GIF describe the old robot, old scene and old rule, so anyone comparing future numbers is comparing across three simultaneous changes. Worth naming all three.

5. hand_over.py:216 keys the gripper default off the arm name. hand_close_qpos = None if args.robot == "ur10" else HAND_CLOSE_QPOS. get_hand_open_close_qpos already recognizes a Robotiq 2F-140 from its six finger/knuckle joint names, so this re-derives the gripper from the arm. A future Robotiq-on-another-arm construction would fall back to the 0.04 PGI value, which is the bug this PR fixes.

Checked and fine. The default-value change is well contained: only six-joint Robotiq hands reach 0.55, and hand_over_benchmark.py is the only benchmark declaring ur10, so the other ten benchmarks and twenty tutorials keep 0.036. The new test file covers boundaries, NaN, inf, negatives and the report path rather than mirroring the implementation. black --check clean on all four files. settled_position is the last hold sample, so the reported 3D diagnostic and the new XY/Z errors come from the same state.

@Yuan-Xinyi Yuan-Xinyi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

see comments above

@matafela

Copy link
Copy Markdown
Collaborator Author

see comments above

I resolve the issue in comments.

@Yuan-Xinyi
Yuan-Xinyi merged commit aa35689 into xinyi/bench-skills Sep 24, 2026
1 check passed
@Yuan-Xinyi
Yuan-Xinyi deleted the cj/bench-skills branch September 24, 2026 05:09
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.

5 participants