Benchmark seven more atomic skills - #642
Open
Yuan-Xinyi wants to merge 6 commits into
Open
Yuan-Xinyi wants to merge 6 commits into
Yuan-Xinyi wants to merge 6 commits into
Conversation
The five shipped atomic-action benchmarks (move_joints, move_end_effector, pick_up, place, move_held_object) could not run as delivered. Each fix: - The tutorial modules never exported create_robot / initialize_simulation (pickup/place/move_held_object also referenced compute_pick_close_end_step and make_pre_pick_eef_pose), so every benchmark failed at import. Added those symbols plus a shared initialize_benchmark_simulation that adapts a benchmark argument namespace to create_tutorial_simulation. - move_joints / move_end_effector compiled without a planning context and hit "IK interpolation requires explicit interpolation_dt"; they now pass initial_context(control_dt=sim.sim_config.physics_dt). - No benchmark released its SimulationManager, so every run (success or failure) hung the process at exit; entry points now wrap main() in run_tutorial for deterministic top-level teardown. - pickup / place / move_held_object called the old two-arg get_hand_open_close_qpos(robot, device) signature. - move_end_effector's 0.01 m success tolerance sat exactly on the resampled-endpoint error (0.0100 m) and failed by micrometres; widened to 0.015 m with an explanatory comment. After the fixes move_joints runs end to end (316 ms plan, success, report written, clean exit) and the mesh-object benchmarks import, plan, replay, and report correctly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sweeping sample_count 80/160/320 across all four pose cases left the endpoint error at exactly 0.0100 m, falsifying the resampling explanation. Axis decomposition shows a constant 1.00 cm offset along the end-effector frame's -X axis, and a direct URSolver probe (get_ik success=True, FK of the solution 1.00 cm from the target) places the discrepancy in the analytic solver vs the tutorial UR5+gripper URDF chain, not in the benchmark or trajectory pipeline. The 0.015 m gate stands until that mismatch is fixed, then 0.01 m should be restored. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Merging main brings in the restructured simulation, where DexSim binds robot.body_data only after SimulationManager.prepare(). Every benchmark entry point reads get_qpos() right after building its robot, so all five failed with "'NoneType' object has no attribute 'qpos'" until prepare() runs. The five tutorial create_robot() helpers now call it. Restore SUCCESS_TOLERANCE_M to 0.01. The 1.00 cm endpoint error that motivated widening it to 0.015 was an artefact of the engine build in use at the time, not of trajectory resampling: on the current engine, with prepare() in place, all four MoveEndEffector cases land at 0.0000-0.0001 m against the original gate. The widened tolerance was masking an environment problem rather than measuring a real one. Verified: move_joints and move_end_effector smoke runs pass, and move_end_effector passes every pose case under the restored 0.01 m gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Per-skill benchmarks so far reported one boolean per case, which cannot
distinguish a plan that was never produced from a plan the robot failed
to track from a plan that ran correctly but did not achieve the task.
Adopt the vocabulary the motion-generation design document already
defines rather than inventing a second one:
planning_success -> motion_valid -> execution_success -> task_success
SuccessLadder records every stage and the reason a case stopped;
check_motion_valid rejects a trajectory that is non-finite or leaves the
joint limits before anything is replayed.
Calibrate the physical replay. Robot.set_qpos writes drive targets, not
state, so replaying a waypoint every few physics steps measures the
controller lagging behind the plan as if it were task error. Sweeping the
rate on OpenDoor, with the arm's own tracking error alongside:
steps/waypoint max tracking error hinge error
1 (plan dt) 1.3844 rad 0.2273 rad
4 (previous) 0.4390 rad 0.0689 rad
16 (chosen) 0.0532 rad 0.0306 rad
32 0.0230 rad 0.0245 rad
Replaying at the planner's own dt is the worst setting, not the best.
DEFAULT_REPLAY_STEPS_PER_WAYPOINT is 16, and execution_success now gates
on max_tracking_error_rad <= 0.10 so no case is credited or blamed for
motion the robot did not perform. On the Slide 180 mm pull the same
trajectory moves from 0.0525 m of error at 1 step to 0.0006 m at 16.
BENCHMARK_STANDARD.md documents the ladder, the per-skill task_success
criteria and where each threshold came from, the failure vocabulary, and
the warm-up requirement for timing.
Everything here is additive: replay_trajectory_for_physical_validation
and its four-step cadence are untouched, so the five existing benchmarks
keep their current behaviour and remain comparable with their published
numbers.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Extend the suite from five skills to twelve, each reporting the full success ladder and a task metric defined by what the skill is supposed to achieve: open_door and slide on the driven joint's travel, press on button depression, twist on knob rotation, pour on the container's tilt, axis_align on the residual axis angle, and hand_over on delivery distance without a drop. Two skills did not run at all, and neither failure was in the benchmarks. Press and Slide plan Cartesian-linear segments, which #640 now requires to use strategy='ik_interp', but their tutorials still request 'motion_gen'; the press tutorial fails on main today with the same error the benchmark hit. Both tutorials and both benchmarks now ask for the strategy the primitives require. Verified by running the press tutorial itself, which completes again. OpenDoor replays at four times the shared rate. At the default the arm trails the plan by 0.66 rad and the tracking gate rejects the run even though the door reaches its commanded angle. Raising only the replay rate, never the task tolerance, brings tracking inside the gate and the hinge to within 0.001 rad. Twist remains a genuine failure and is reported as one. The planned end-effector rotation about the knob axis is exactly the commanded 0.7854 rad with correct sign at 0.026 rad of arm tracking error, so the skill plans correctly; the gripper wedges against the knob instead of holding it, and the knob joint has near-zero stiffness, so it over-rotates and keeps turning during retract. Neither the gate nor the scene stiffness was adjusted to hide this. BENCHMARK_REPORT.md records the measured ladder, timings and task values per skill, the bugs found and fixed, and the threshold questions that need a decision rather than a default. Not built: push_object has no tutorial to source a scene from, and the two coordinated dual-arm skills are left for a follow-up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+46
to
+52
| "open_door": "scripts.benchmark.atomic_action.open_door_benchmark", | ||
| "press": "scripts.benchmark.atomic_action.press_benchmark", | ||
| "slide": "scripts.benchmark.atomic_action.slide_benchmark", | ||
| "twist": "scripts.benchmark.atomic_action.twist_benchmark", | ||
| "axis_align": "scripts.benchmark.atomic_action.axis_align_benchmark", | ||
| "pour": "scripts.benchmark.atomic_action.pour_benchmark", | ||
| "hand_over": "scripts.benchmark.atomic_action.hand_over_benchmark", |
There was a problem hiding this comment.
The new actions cannot run through --in_process: _make_child_args() omits their case-selection fields, but each new run_all_benchmarks() accesses its field directly before applying the smoke profile. For example, --action open_door --in_process --profile smoke raises AttributeError on args.door_cases instead of producing a report. Populate the new action-specific defaults when constructing the child namespace, preferably using each module's argument parser.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/benchmark/atomic_action/run_benchmark.py
Line: 46-52
Comment:
**In-process actions lack required arguments**
The new actions cannot run through `--in_process`: `_make_child_args()` omits their case-selection fields, but each new `run_all_benchmarks()` accesses its field directly before applying the smoke profile. For example, `--action open_door --in_process --profile smoke` raises `AttributeError` on `args.door_cases` instead of producing a report. Populate the new action-specific defaults when constructing the child namespace, preferably using each module's argument parser.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+146
to
+150
| unit_axis = axis / torch.linalg.vector_norm(axis) | ||
| sign = torch.sign(torch.dot(vector, unit_axis.to(vector.dtype))) | ||
| if float(sign) == 0.0: | ||
| sign = torch.ones_like(sign) | ||
| return float(angle * sign) |
There was a problem hiding this comment.
This helper returns the total relative rotation angle; the requested axis only determines its sign. A 45-degree tumble perpendicular to the pour axis therefore returns positive 45 degrees because the zero dot product is forced to a positive sign. The 45-degree benchmark can consequently credit an off-axis slip as a successful pour. Measure rotation about the requested axis and add a perpendicular-axis counterexample test.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/benchmark/atomic_action/pour_benchmark.py
Line: 146-150
Comment:
**Off-axis tumbles count as pours**
This helper returns the total relative rotation angle; the requested axis only determines its sign. A 45-degree tumble perpendicular to the pour axis therefore returns positive 45 degrees because the zero dot product is forced to a positive sign. The 45-degree benchmark can consequently credit an off-axis slip as a successful pour. Measure rotation about the requested axis and add a perpendicular-axis counterexample test.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+294
to
+296
| rotation_error = abs( | ||
| abs(trace.peak_signed_displacement) - abs(case.rotate_angle_rad) | ||
| ) |
There was a problem hiding this comment.
Taking the absolute value of both rotations removes the direction check from the signed task metric. If a slipping object rotates −45 degrees around the requested axis for the +45-degree case, this reports zero error and task_success=True. The measurement and primitive use the same axis convention, so compare the signed achieved rotation with the signed command rather than their magnitudes.
Suggested change
| rotation_error = abs( | |
| abs(trace.peak_signed_displacement) - abs(case.rotate_angle_rad) | |
| ) | |
| rotation_error = abs( | |
| trace.peak_signed_displacement - case.rotate_angle_rad | |
| ) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/benchmark/atomic_action/pour_benchmark.py
Line: 294-296
Comment:
**Reversed pours pass the gate**
Taking the absolute value of both rotations removes the direction check from the signed task metric. If a slipping object rotates −45 degrees around the requested axis for the +45-degree case, this reports zero error and `task_success=True`. The measurement and primitive use the same axis convention, so compare the signed achieved rotation with the signed command rather than their magnitudes.
```suggestion
rotation_error = abs(
trace.peak_signed_displacement - case.rotate_angle_rad
)
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+238
to
+247
| min_z = state["min_z"] if state["min_z"] != float("inf") else None | ||
| dropped = min_z is not None and min_z < HANDOVER_DROP_Z_M | ||
| if trace is not None and ( | ||
| trace.max_tracking_error_rad > REPLAY_TRACKING_TOLERANCE_RAD | ||
| ): | ||
| ladder.execution_success = False | ||
| ladder.fail("execution_success", "controller_tracking_failure") | ||
| elif ladder.execution_success and trace is not None: | ||
| delivered = trace.settled_position <= HANDOVER_DELIVERY_TOLERANCE_M | ||
| ladder.task_success = delivered and not dropped |
There was a problem hiding this comment.
The drop threshold is below the support table, so it cannot detect an object dropped onto that table during transfer. The target lies inside the table footprint, only 0.10 m above its surface. An object resting near the target at z≈0.53 m passes both the 0.12 m delivery tolerance and the z≥0.35 m check despite losing its grasp prematurely. Track grasp retention or an unexpected return to the support surface during transfer, while allowing the primitive's intentional final release.
Knowledge Base Used: Simulation lab
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/benchmark/atomic_action/hand_over_benchmark.py
Line: 238-247
Comment:
**Tabletop drops escape detection**
The drop threshold is below the support table, so it cannot detect an object dropped onto that table during transfer. The target lies inside the table footprint, only 0.10 m above its surface. An object resting near the target at z≈0.53 m passes both the 0.12 m delivery tolerance and the z≥0.35 m check despite losing its grasp prematurely. Track grasp retention or an unexpected return to the support surface during transfer, while allowing the primitive's intentional final release.
**Knowledge Base Used:** [Simulation lab](https://app.greptile.com/dexforce/-/custom-context/knowledge-base/dexforce/embodichain/-/docs/simulation-lab.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+286
to
+291
| recorded = replay_trajectory_with_recording( | ||
| sim=sim, | ||
| robot=robot, | ||
| traj=traj, | ||
| args=args, | ||
| video_path=build_video_output_path( |
There was a problem hiding this comment.
The recorder uses four physics steps per waypoint, whereas OpenDoor is scored at 64 and the other new benchmarks at 16. For these contact tasks, that changes tracking and achieved motion—not just playback speed—so a video attached to a successful case can depict a different outcome. Record during the measured replay or give the recording path the same replay cadence and terminal hold settings.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/benchmark/atomic_action/open_door_benchmark.py
Line: 286-291
Comment:
**Videos replay a different execution**
The recorder uses four physics steps per waypoint, whereas OpenDoor is scored at 64 and the other new benchmarks at 16. For these contact tasks, that changes tracking and achieved motion—not just playback speed—so a video attached to a successful case can depict a different outcome. Record during the measured replay or give the recording path the same replay cadence and terminal hold settings.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+318
to
+319
| video_path = str(recorded) if recorded is not None else "" | ||
| video_paths.append(video_path) |
There was a problem hiding this comment.
A failed or unavailable recording is appended to video_paths as an empty string. With --record_video --record_failed_video and the default case limit, a planning failure with no trajectory can consume the quota, suppress a later recordable case, and inflate the reported video count. Append only nonempty recording paths, as the other new benchmarks do. Twist has the same issue.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/benchmark/atomic_action/slide_benchmark.py
Line: 318-319
Comment:
**Failed recordings consume video quota**
A failed or unavailable recording is appended to `video_paths` as an empty string. With `--record_video --record_failed_video` and the default case limit, a planning failure with no trajectory can consume the quota, suppress a later recordable case, and inflate the reported video count. Append only nonempty recording paths, as the other new benchmarks do. Twist has the same issue.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+329
to
+330
| state["pour_start_z"], | ||
| float(obj.get_local_pose(to_matrix=True)[0, 2, 3]), |
There was a problem hiding this comment.
final_object_z_m is read after the optional reset and recording replay, while the rotation trace and starting height describe the scored run. Enabling recording can therefore replace this diagnostic with the height from a different execution—or the reset pose if recording cannot start. Capture the final height immediately after physical validation, before resetting the scene for video.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/benchmark/atomic_action/pour_benchmark.py
Line: 329-330
Comment:
**Recording changes the height diagnostic**
`final_object_z_m` is read after the optional reset and recording replay, while the rotation trace and starting height describe the scored run. Enabling recording can therefore replace this diagnostic with the height from a different execution—or the reset pose if recording cannot start. Capture the final height immediately after physical validation, before resetting the scene for video.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| return [AXIS_ALIGN_CASES[name] for name in case_names] | ||
|
|
||
|
|
||
| def object_axis_angle_rad(obj, internal_axis, target_axis) -> float: |
There was a problem hiding this comment.
The exported object_axis_angle_rad() leaves all three parameters unannotated, violating the repository directive to fully annotate public APIs. Add RigidObject and tensor parameter annotations, using type-only imports where appropriate. The exported signed_rotation_about_axis_rad() in Pour has the same omission. This repository requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/benchmark/atomic_action/axis_align_benchmark.py
Line: 115
Comment:
**Exported helpers lack parameter annotations**
The exported `object_axis_angle_rad()` leaves all three parameters unannotated, violating the repository directive to fully annotate public APIs. Add `RigidObject` and tensor parameter annotations, using type-only imports where appropriate. The exported `signed_rotation_about_axis_rad()` in Pour has the same omission. This repository requirement must be satisfied before merging.
**Context Used:** CLAUDE.md ([source](https://github.com/dexforce/embodichain/blob/main/CLAUDE.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+73
to
+86
| actually got there. Sweeping the rate on OpenDoor: | ||
|
|
||
| | Steps/waypoint | Max arm tracking error (rad) | Hinge error (rad) | | ||
| |---|---|---| | ||
| | 1 (planner's own dt) | 1.3844 | 0.2273 | | ||
| | 4 (original) | 0.4390 | 0.0689 | | ||
| | 16 (**chosen**) | 0.0532 | 0.0306 | | ||
| | 32 | 0.0230 | 0.0245 | | ||
|
|
||
| The measured "task" error was largely the controller failing to follow the plan. | ||
| I first tried replaying at the planner's own `dt`, which is *worse* — it made | ||
| OpenDoor and Slide fail outright. | ||
|
|
||
| **Fix**: `DEFAULT_REPLAY_STEPS_PER_WAYPOINT = 16`, plus a new |
There was a problem hiding this comment.
The report presents 16 steps per waypoint as OpenDoor's chosen replay rate, but the committed benchmark uses 64 because 16 fails its tracking gate. Together with the opening claim that the changes are uncommitted, this leaves readers unable to tie the reported results to the reviewed implementation. Identify the measured revision and settings, and separate historical experiments from the current benchmark configuration.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/benchmark/atomic_action/BENCHMARK_REPORT.md
Line: 73-86
Comment:
**Report describes outdated replay settings**
The report presents 16 steps per waypoint as OpenDoor's chosen replay rate, but the committed benchmark uses 64 because 16 fails its tracking gate. Together with the opening claim that the changes are uncommitted, this leaves readers unable to tie the reported results to the reviewed implementation. Identify the measured revision and settings, and separate historical experiments from the current benchmark configuration.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
matafela
previously approved these changes
Sep 18, 2026
matafela
self-requested a review
September 18, 2026 02:14
matafela
requested changes
Sep 18, 2026
| "pick_up": "scripts.benchmark.atomic_action.pickup_benchmark", | ||
| "move_held_object": "scripts.benchmark.atomic_action.move_held_object_benchmark", | ||
| "place": "scripts.benchmark.atomic_action.place_benchmark", | ||
| "open_door": "scripts.benchmark.atomic_action.open_door_benchmark", |
Collaborator
There was a problem hiding this comment.
[P2] Initialize action-specific defaults for in-process dispatch
The newly registered actions are also exposed through --in_process, but _make_child_args() does not populate their required case-selection attributes, such as door_cases, press_cases, and align_cases. Each new benchmark reads its attribute directly, causing an AttributeError; all seven new actions are affected, including the smoke profile. Please initialize the child namespace through the selected module’s add_benchmark_args() defaults before applying shared overrides, and cover this dispatch path with a focused regression check.
Yuan-Xinyi
force-pushed
the
xinyi/bench-standard
branch
from
September 21, 2026 08:30
13cf9bb to
b0eece2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Third of three stacked changes. Base is #641, which is itself based on #620; review those first.
Extends the suite from five skills to twelve. Each new benchmark reports the full success ladder from #641 and a task metric defined by what the skill is supposed to achieve, not by whether a planner returned:
task_successis measured asopen_doorpressslidepouraxis_alignhand_overtwistTwo skills did not run at all, and neither failure was in the benchmarks
Press and Slide plan Cartesian-linear segments. #640 now requires those to use
strategy='ik_interp', but their tutorials still request'motion_gen', so the guard rejects them. This is not benchmark-only: the press tutorial fails onmaintoday with exactly the error the benchmark hit —Both tutorials and both benchmarks now request the strategy the primitives require. Verified by running the press tutorial itself, which completes again.
OpenDoor: the replay rate, never the task tolerance
At the shared default the arm trails the plan by 0.66 rad and the tracking gate rejects the run, even though the door reaches its commanded angle. Raising only the replay rate — four times the shared default for this skill — brings tracking inside the gate and the hinge to within 0.001 rad. The task tolerance is unchanged.
Twist is a real failure and is reported as one
The planned end-effector rotation about the knob axis is exactly the commanded 0.7854 rad, with correct sign, at 0.026 rad of arm tracking error — so the skill plans correctly. The gripper wedges against the knob rather than holding it, and the knob joint has near-zero stiffness, so the knob over-rotates and keeps being dragged during retract. Neither the success gate nor the scene stiffness was adjusted to turn this green;
BENCHMARK_REPORT.mdproposes three ways to gate it and recommends one.Type of change
press/slidestrategy regression)Screenshots
N/A — reports are Markdown tables written to
outputs/benchmarks/.Checklist
black .command to format the code base.Validation
Every new benchmark run on this branch, current engine:
BENCHMARK_REPORT.mdrecords the measured ladder, timings and task values per skill, the bugs found and fixed, and the threshold questions that need a decision rather than a default — including one that matters:press's 80 %-of-stroke rule passes regardless of commanded depth on this asset, so it currently proves only that the button was pressed.Not built:
push_objecthas no tutorial to source a scene from, and the two coordinated dual-arm skills are left for a follow-up.