Add the atomic-action benchmark measurement standard - #641
Open
Yuan-Xinyi wants to merge 5 commits into
Open
Yuan-Xinyi wants to merge 5 commits into
Yuan-Xinyi wants to merge 5 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>
7 tasks
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1202
to
+1205
| if traj is None or getattr(traj, "ndim", 0) < 3 or traj.shape[1] == 0: | ||
| return False, "non_finite_trajectory" | ||
| if not bool(torch.isfinite(traj).all()): | ||
| return False, "non_finite_trajectory" |
There was a problem hiding this comment.
The new trajectory validation, replay tracking, and success-ladder behavior has no focused test or in-repository caller. The reported smoke run exercises an unchanged benchmark path, leaving cases such as non-finite trajectories, limit violations, tracking failures, and ladder transitions unverified. This violates the repository directive that new features include focused tests proving their behavior, so the requirement must be satisfied before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/benchmark/atomic_action/common.py
Line: 1202-1205
Comment:
**Focused tests are missing**
The new trajectory validation, replay tracking, and success-ladder behavior has no focused test or in-repository caller. The reported smoke run exercises an unchanged benchmark path, leaving cases such as non-finite trajectories, limit violations, tracking failures, and ladder transitions unverified. This violates the repository directive that new features include focused tests proving their behavior, so the requirement must be satisfied before merging.
**Context Used:** AGENTS.md ([source](https://github.com/dexforce/embodichain/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+1529
to
+1532
| try: | ||
| measure_waypoint = result.segment(0, actuation_segment).stop - 1 | ||
| except (KeyError, AttributeError, IndexError): | ||
| measure_waypoint = int(traj.shape[1]) - 1 |
There was a problem hiding this comment.
A missing or misspelled actuation_segment is silently replaced with the trajectory's final waypoint. For contact plans that release and retract, this scores the target after control has ended—the exact rebound condition this helper is intended to avoid. The benchmark can therefore report a misleading task result instead of identifying invalid segment metadata.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/benchmark/atomic_action/common.py
Line: 1529-1532
Comment:
**Missing segments are mis-scored**
A missing or misspelled `actuation_segment` is silently replaced with the trajectory's final waypoint. For contact plans that release and retract, this scores the target after control has ended—the exact rebound condition this helper is intended to avoid. The benchmark can therefore report a misleading task result instead of identifying invalid segment metadata.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+1345
to
+1347
| step_counts = waypoint_step_counts( | ||
| waypoint_dt, physics_dt, waypoint_count, steps_per_waypoint | ||
| ) |
There was a problem hiding this comment.
The advertised waypoint_dt mode replays at planner timing, while the new benchmark standard establishes that this cadence produces tracking errors above one radian and invalidates contact measurements. A caller using these documented parameters will trigger controller failures instead of using the calibrated 16-step replay. The mode should be removed or made consistent with the standard.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/benchmark/atomic_action/common.py
Line: 1345-1347
Comment:
**Timing mode breaks calibration**
The advertised `waypoint_dt` mode replays at planner timing, while the new benchmark standard establishes that this cadence produces tracking errors above one radian and invalidates contact measurements. A caller using these documented parameters will trigger controller failures instead of using the calibrated 16-step replay. The mode should be removed or made consistent with the standard.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
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
1 participant
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
Second of three stacked changes for the atomic-action benchmark suite. Base is #620; review that one first.
Per-skill benchmarks report one boolean per case today, which cannot distinguish three different outcomes: a plan that was never produced, a plan the robot failed to track, and a plan that ran correctly but did not achieve the task. They are different bugs with different owners, so this change records them separately, reusing the vocabulary
scripts/benchmark/motion_generation/BENCHMARK_DESIGN.mdalready defines rather than inventing a second one:SuccessLadderrecords every stage and the reason a case stopped.check_motion_validrejects a non-finite trajectory, or one that leaves the joint limits, before anything is replayed.BENCHMARK_STANDARD.mddocuments the ladder, the per-skilltask_successcriteria with the origin of every threshold, the failure vocabulary, and the warm-up requirement for timing.The replay rate was measuring the wrong thing
Robot.set_qposwrites drive targets, not state. Replaying one waypoint every few physics steps therefore 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 the task metric:dt)Replaying at the planner's own
dtis the worst setting, not the best — worth stating explicitly, because it is the intuitive choice. 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.execution_successnow gates onmax_tracking_error_rad <= 0.10, so no case is credited or blamed for motion the robot did not perform.Type of change
Screenshots
N/A — reports are Markdown tables written to
outputs/benchmarks/.Checklist
black .command to format the code base.Validation
common.pyis +547 / −0.replay_trajectory_for_physical_validationand its four-step cadence are untouched, so the five existing benchmarks keep their current behaviour and stay comparable with their published numbers.