Define the atomic-action benchmark measurement standard - #641
Conversation
|
| 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!
| 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.
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.
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.| step_counts = waypoint_step_counts( | ||
| waypoint_dt, physics_dt, waypoint_count, steps_per_waypoint | ||
| ) |
There was a problem hiding this 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.
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.Per-skill benchmarks report one boolean per case today, which cannot say whether a plan was never produced, the robot never got to the object, or the skill reached the object and missed the goal. They are different problems with different owners. Define the standard first, as a document, before changing any benchmark: - Three dimensions: success_rate, plus robustness and adaptability as success_rate recomputed over a perturbation and a generalization case set. - success_rate is an ordered list of stages per skill, each a scene measurement taken at the end of the segment where the skill stops acting on what the stage is about. A stage is reached only when every earlier stage passed, so the report localizes the failure. Closing stages count: a skill that lifts an object and then drops it did not pick it up. - Two tolerance sets, split by binding contract rather than per skill. MoveEndEffector and MoveJoints bind only a motion endpoint and are scored after the terminal settle with the drive converged (3 mm, 0.3 deg); the other thirteen also bind a grasp endpoint and are scored mid-trajectory with the arm contact-loaded (1 cm, 5 deg). Every value is 1.8x the worst measured error for its set. Trajectory-level and controller-level properties are excluded on purpose: they belong to scripts/benchmark/motion_generation and are reported here as diagnostics only. A skill that opened the door is not failed because the drive lagged the plan. Tolerances are calibrated by physical replay. The shipped MoveJoints and MoveEndEffector criteria read traj[:, -1] and report 0.000000 rad and 0.00002 m, which describes the solver rather than the robot; replaying the same coverage cases in physics gives 0.00294 rad and 1.54 mm. The failure vocabulary is the task-level subset of the taxonomy in motion_generation/BENCHMARK_DESIGN.md section 7, used verbatim. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
13cf9bb to
b0eece2
Compare
| These benchmarks measure **skills**, not trajectories. Whether a plan is finite, | ||
| stays inside the joint limits, avoids collisions, or is tracked faithfully by | ||
| the drive belongs to `scripts/benchmark/motion_generation/`. Those properties | ||
| are reported here as diagnostic columns only; none of them fails a skill. |
There was a problem hiding this comment.
The standard says motion validity and faithful tracking are diagnostic only and cannot fail a skill. That conflicts with the referenced motion-generation success ladder, where task_success requires both motion_valid and execution_success, and with the PR description's max_tracking_error_rad <= 0.10 gate. Under this definition, an unsafe trajectory or motion the robot did not perform can still be credited as a successful skill.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/benchmark/atomic_action/BENCHMARK_STANDARD.md
Line: 3-6
Comment:
**Invalid Motion Can Pass**
The standard says motion validity and faithful tracking are diagnostic only and cannot fail a skill. That conflicts with the referenced motion-generation success ladder, where `task_success` requires both `motion_valid` and `execution_success`, and with the PR description's `max_tracking_error_rad <= 0.10` gate. Under this definition, an unsafe trajectory or motion the robot did not perform can still be credited as a successful skill.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| ## 1. success_rate | ||
|
|
||
| Each skill declares ordered **stages**, each a scene measurement taken at the | ||
| end of the segment where the skill stops acting on what the stage is about. A | ||
| stage is reached only when every earlier stage passed. | ||
|
|
||
| ``` | ||
| stage_success_rate[i] = passed(i) / reached(i) reached(i) = passed(i-1) | ||
| success_rate = passed(last) / total | ||
| ``` |
There was a problem hiding this comment.
Standard Describes Unimplemented Behavior
The document presents staged rates, failure-stage metadata, replacement tolerances, 16-step replay, and max_tracking_error_rad reporting as current behavior. The shipped benchmarks still produce one success boolean with legacy reasons and tolerances, replay four physics steps per waypoint, and do not compute the promised tracking diagnostic. For example, Place still uses a 10 cm tolerance while this standard specifies 1 cm. As a result, existing reports cannot be reproduced or interpreted according to this standard.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/benchmark/atomic_action/BENCHMARK_STANDARD.md
Line: 58-67
Comment:
**Standard Describes Unimplemented Behavior**
The document presents staged rates, failure-stage metadata, replacement tolerances, 16-step replay, and `max_tracking_error_rad` reporting as current behavior. The shipped benchmarks still produce one success boolean with legacy reasons and tolerances, replay four physics steps per waypoint, and do not compute the promised tracking diagnostic. For example, Place still uses a 10 cm tolerance while this standard specifies 1 cm. As a result, existing reports cannot be reproduced or interpreted according to this standard.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.034818d to
b0eece2
Compare
|
The adaptability section currently lists generalization axes, but it does not yet define a testable generalization target. In particular, it does not freeze a held-out domain set, state what skill semantics must remain invariant, or define coverage and pass thresholds. A concise addition could be:
This keeps the standard compact while turning the current axis list into a comparable and reproducible generalization objective. |
The adaptability section listed generalization axes but no target: nothing froze the held-out set, said what must stay invariant, or defined a pass rule. An implementation could drop the hard domains and still report a valid-looking minimum. - Separate the two dimensions: robustness perturbs conditions inside the nominal domain, adaptability leaves it for held-out objects, scenes or embodiments outside the calibration domain. - Measure on a frozen, versioned domain set that did not inform case selection, tolerances or skill parameters. Every case preserves the semantic goal and the binding contract, so section 1 applies unchanged -- same stages, same tolerance set, no per-domain recalibration. - Report adaptability_coverage_rate, adaptability_mean, adaptability_worst, adaptability_variance and adaptability_retention against the nominal rate. - Eligibility requires 100 % coverage of mandatory cases plus suite-owned, versioned thresholds on the worst domain and on retention. Mean and variance are reported but do not decide eligibility. - With nominal_success_rate at 0, retention is N/A and eligibility rests on the worst-domain score; that state already fails section 1. Vocabulary follows motion_generation/BENCHMARK_DESIGN.md section 5, which this document already borrows from: coverage_rate, eligible, mandatory, applicable, required_capabilities and N/A rather than new words for the same things. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
90f12e7 to
5d84c56
Compare
|
@yuecideng Done in 5d84c56 — section 3 rewritten along your four points.
Two deviations from your wording, both to avoid a second vocabulary for
One addition on top of your text: when Still deliberately unset, and now stated that way in Open item 3: which meshes, |
Description
Defines how atomic-skill benchmarks measure a skill, as a document only. No
benchmark code changes — the implementation follows in a separate change, so
the standard can be argued about before anything is built on it.
scripts/benchmark/atomic_action/BENCHMARK_STANDARD.md, +158, nothing else.The problem
Per-skill benchmarks report one boolean per case. That cannot distinguish a
plan that was never produced, a robot that never reached the object, and a
skill that reached the object and missed the goal. They are different problems
with different owners.
What the standard says
Three dimensions.
success_rateis specified here.robustnessandadaptabilityaresuccess_raterecomputed over a perturbation set and ageneralization set, so they inherit every definition.
robustnesshas its axeslisted and its magnitudes unfixed.
adaptabilitynow has a target as well: afrozen, versioned held-out domain set that did not inform case selection,
tolerances or skill parameters, scored per domain and reported as
adaptability_coverage_rate,adaptability_mean,adaptability_worst,adaptability_varianceandadaptability_retention, with eligibility at 100 %coverage of mandatory cases plus suite-owned thresholds on the worst domain and
on retention. The domains and the two threshold values land with the suite.
success_rate is a per-skill ordered list of stages. Each stage is a scene
measurement taken at the end of the segment where the skill stops acting on
what the stage is about. A stage is reached only when every earlier stage
passed, so a report localizes the failure instead of collapsing it:
Closing stages count. A skill that lifts an object and then drops it did not
pick it up. All 15 built-in skills have stages defined, including the three
that have no benchmark module yet.
Two tolerance sets, split by binding contract — not per skill.
MoveEndEffectorandMoveJointsbind only amotionendpoint and are scoredafter the terminal settle with the drive converged; the other thirteen also
bind a
graspendpoint and are scored mid-trajectory, with the armcontact-loaded. That is a physical difference, not a preference:
PRIMITIVE_*TASK_*Every value is 1.8x the worst error measured for its set. Each pair describes
one physical error at a characteristic lever arm: 0.57 m (arm reach) and
0.115 m (handle, drawer front, grasped object). Tolerances are absolute, never
scaled by commanded magnitude, so cases stay comparable.
A skill that breaks and re-establishes contact gets a derived budget rather
than its own constant:
n * TASK_POSITION_TOLERANCE_Mforncontact phases.HandOver has three, so 3 cm — it measures 11.2 cm and therefore fails, at the
placedstage, withgrasped,transferredandhanded_overall passing.That is the decomposition doing its job, not a tolerance to relax.
Trajectory-level criteria are excluded on purpose
Whether a plan is finite, stays inside the joint limits, avoids collisions, or
is tracked faithfully by the drive belongs to
scripts/benchmark/motion_generation/, whose own design document labels thatlayer
L1 trajectory. Here those properties are diagnostic columns only. Askill that opened the door is not failed because the drive lagged the plan.
max_tracking_error_radis still reported per case, so a reader can judgewhether a scene measurement is trustworthy.
Tolerances are calibrated against physics, not against the planner
The shipped
MoveJointsandMoveEndEffectorcriteria compute their errorfrom
traj[:, -1]— the planner's own endpoint, with no physics. They report0.000000rad and0.00002m, which describes the solver, not the robot.Replaying the same coverage cases in physics and reading the achieved state:
traj[:, -1]MoveJointsMoveEndEffectornear_centerfront_leftfront_rightfar_centerHence 3 mm / 0.3°, not the 0.01 m / 1e-4 rad in the shipped benchmarks.
For
TASK_*, the worst measured errors areSlide0.0056 m andAxisAlign0.0480 rad (2.75°), against a contact-loaded arm tracking error of 0.0532 rad
(3.05°). A task rotation tolerance below about 3° is unreachable in this
simulation at any replay rate, because the arm itself lags the plan by that
much while loaded.
Vocabulary
The failure reasons are the task-level subset of the taxonomy in
motion_generation/BENCHMARK_DESIGN.mdsection 7, used verbatim.stageandpeak signedare that document's words too. The two tolerance sets are namedafter their constants rather than a new tier vocabulary, and
PHYSICAL_PICK_MIN_LIFT_Mis the existing constant, reused.TASK_POSITION_TOLERANCE_MreplacesPHYSICAL_PLACE_XY_TOLERANCE_M(0.10) andPHYSICAL_MOVE_HELD_OBJECT_XYZ_TOLERANCE_M(0.12).Type of change
Screenshots
N/A
Checklist
black .command to format the code base (no Python changed)Validation
Rebased onto current
main, so the earlier revision's incidental diff is gone:ten of its twelve non-document files were already identical to
mainand onlyappeared because the branch predated #620. Two others would have removed
affordance_samplingsupport that landed onmainafter the branch point(
parse_affordance_sampling_arguments,create_affordance_sampling_context,log_affordance_branch_diagnosticsand their CLI). The branch is now twocommits adding a single file.
Calibration runs behind the
PRIMITIVE_*numbers:plus a physical-replay pass over the same cases at 16 steps per waypoint with a
terminal settle, which is what produced the right-hand column above.
Open
Twist'stwistedstage gates on the executed end-effector rotationabout the knob axis, which measures exactly 0.7854 rad for a 0.7854 rad
command. The knob joint itself over-rotates to 2.38–3.14 rad for
0.52–1.57 rad commanded, because the gripper wedges against the 5 cm knob
and the joint offers no resistance; it is reported as a diagnostic. The
alternative was to keep gating on the knob joint and let
Twistfail onthis asset.
Press's criterion only shows that the button moved. Commanding 3 mmstill drives it to 5.66 mm of its 6 mm stroke, so the 80 % ratio does not
discriminate commanded depth on this asset.
robustnessmagnitudes, andadaptability's domains and thresholds, arenot set. For
robustnessthe axes are listed and how much to perturb isnot decided.
adaptabilityis defined above, but whichmeshes, assets, layouts, end effectors and arms make up version 1 of the
held-out set, and what
adaptability_worstandadaptability_retentionmust reach, are owned by the suite and land with it.
PRIMITIVE_*rotation is calibrated at one terminal pose. All threeMoveJointscoverage cases end athome, and all three settle to the same0.002944 rad — a systematic steady-state drive offset, reproducible but
single-pose.
--repeat 1.🤖 Generated with Claude Code