Skip to content

Evaluate terminal Atomic Skills with the v1 standard - #773

Closed
Yuan-Xinyi wants to merge 1 commit into
xinyi/pr712-with-mainfrom
feat/atomic-standard-terminal-evaluators
Closed

Yuan-Xinyi wants to merge 1 commit into
xinyi/pr712-with-mainfrom
feat/atomic-standard-terminal-evaluators

Conversation

@Yuan-Xinyi

Copy link
Copy Markdown
Collaborator

Stack

Description

This PR reuses the parts of #642 that directly strengthen #712 without moving the full legacy benchmark suite.

It adds a batched-evidence bridge that applies the v1 criteria row by row, then migrates MoveEndEffector and MoveJoints to PhysicalStageEvaluator. Both skills are now scored from terminal settled simulation state rather than planner output:

  • MoveEndEffector: measured TCP position error, using the v1 3 mm criterion.
  • MoveJoints: measured joint L2 error, using the v1 0.00524 rad criterion.

Each result records explicit simulation_state provenance. A missing terminal joint observation becomes evidence_unavailable rather than success. Skills that need contact or intermediate-segment observations remain on the legacy task_result() path; this PR does not fabricate those measurements.

Refs #642 and #712.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which improves an existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (existing functionality will not work without user modification)
  • Documentation update

Screenshots

N/A.

Checklist

  • I have run the black . command to format the code base.
  • I reviewed affected documentation and agent context, updated it where needed, or explained why no update was needed.
    • The benchmark README now documents the two migrated providers.
    • context.py affected reports no affected project-context topic for these benchmark files.
  • Public API changes are reflected in the API docs (python docs/scripts/check_api_docs.py), if applicable.
    • No public embodichain exports changed; the gate reports 2487/2487.
  • I have added tests that prove the behavior.
  • Dependencies have been updated, if applicable.
    • No dependency changes.

Validation

black --check --diff --color ./
  1233 files would be left unchanged
python docs/scripts/check_api_docs.py
  2487/2487 exports documented
pytest -q tests/benchmark/motion_generation/test_contracts.py \
  tests/benchmark/motion_generation/test_evaluation.py
  56 passed
python -m py_compile <four changed Python files>
  passed
git diff --check
  passed

tests/benchmark/motion_generation/test_atomic_task_benchmark.py cannot be collected in the available local environment because its installed dexsim.types lacks the current RTRenderMode export. The failure occurs during package import before test execution; the changed test file passes py_compile.

@Yuan-Xinyi Yuan-Xinyi added enhancement New feature or request motion gen Things related to motion generation for robot atomic action atomic action related functionality labels Oct 13, 2026
@greptile-apps

greptile-apps Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[Medium impact] Adds batch evaluation for terminal Atomic Skills.

Fix nonfinite-row handling before merging so one unstable environment cannot invalidate healthy rows.

Fix All in CodexFindings

  1. P1 One bad row fails everyone ▶
Fix with agent prompt
### Issue 1
scripts/benchmark/motion_generation/scenarios/atomic_task.py:605-614
If one replay row ends with NaN or infinite joint positions, `_single_stage_evidence_batch` passes its nonfinite error into `StageEvidence`, which raises `ValueError`. Both migrated skills evaluate every row before applying `execution_success`, so that mask cannot protect healthy rows. The runner then records `metric_evaluation_error` and marks the entire batch failed, losing the healthy rows' measured results.

Return empty evidence for each nonfinite measurement and keep evaluating finite rows. Add a mixed finite/nonfinite batch test.

---

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

Summary

This PR scores MoveEndEffector and MoveJoints from settled simulation observations using the v1 criteria.

  • MoveEndEffector scores the settled TCP position under v1.
  • MoveJoints scores the settled arm joints under v1.

Intentional changes acknowledged by Yuan-Xinyi: the PR description specifies the 3 mm position criterion and the 0.00524 rad joint L2 criterion. The changed thresholds, joint measure, and position-only physical criterion were not reported as defects.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Settled simulation observations] --> B{Skill}
  B --> C[MoveEndEffector position error]
  B --> D[MoveJoints joint L2 error]
  D --> E{Joint observation available?}
  E -->|No| F[Unavailable evidence]
  E -->|Yes| G[Build row evidence]
  C --> G
  G --> H[Apply v1 criteria per row]
  F --> H
  H --> I[Require execution success and valid motion]
  I --> J[Record outcomes]
  G -->|Nonfinite error| K[Exception replaces whole batch with failures]
Loading

Reviews (1) · Last reviewed commit: "feat(benchmark): evaluate terminal atomi..." · Reviewed by Greptile

Comment on lines +605 to +614
return tuple(
{
stage_id: StageEvidence(
window=window,
source="simulation_state",
measurements={measurement: float(value.item())},
method=method,
)
}
for value in values

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 One bad row fails everyone

If one replay row ends with NaN or infinite joint positions, _single_stage_evidence_batch passes its nonfinite error into StageEvidence, which raises ValueError. Both migrated skills evaluate every row before applying execution_success, so that mask cannot protect healthy rows. The runner then records metric_evaluation_error and marks the entire batch failed, losing the healthy rows' measured results.

Return empty evidence for each nonfinite measurement and keep evaluating finite rows. Add a mixed finite/nonfinite batch test.

Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/benchmark/motion_generation/scenarios/atomic_task.py
Line: 605-614

Comment:
**One bad row fails everyone**

If one replay row ends with NaN or infinite joint positions, `_single_stage_evidence_batch` passes its nonfinite error into `StageEvidence`, which raises `ValueError`. Both migrated skills evaluate every row before applying `execution_success`, so that mask cannot protect healthy rows. The runner then records `metric_evaluation_error` and marks the entire batch failed, losing the healthy rows' measured results.

Return empty evidence for each nonfinite measurement and keep evaluating finite rows. Add a mixed finite/nonfinite batch test.

---

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

@Yuan-Xinyi

Copy link
Copy Markdown
Collaborator Author

Closing this PR because it implemented new v1 evaluator glue rather than migrating reusable components from #642. I will replace it with a focused component migration.

@Yuan-Xinyi Yuan-Xinyi closed this Oct 13, 2026
@Yuan-Xinyi
Yuan-Xinyi deleted the feat/atomic-standard-terminal-evaluators branch October 13, 2026 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

atomic action atomic action related functionality enhancement New feature or request motion gen Things related to motion generation for robot

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant