Measure dynamic limits and mark nominal timing in the NMG benchmark by Yuan-Xinyi · Pull Request #713 · DexForce/EmbodiChain

Yuan-Xinyi · 2026-09-28T14:37:50Z

Stack

Stack: main ← Replace external TOPPRA with differentiable CPU/CUDA retiming #724 ← Measure dynamic limits and mark nominal timing in the NMG benchmark #713 ← Retime NMG rollouts under joint velocity and acceleration limits #714 ← Pair a nominal and a retimed NMG arm in the benchmark #715
Base: feat/differentiable-toppra (Replace external TOPPRA with differentiable CPU/CUDA retiming #724)
Depends on: Replace external TOPPRA with differentiable CPU/CUDA retiming #724. This layer does not use it directly; it sits here so Retime NMG rollouts under joint velocity and acceleration limits #714 can retime with Replace external TOPPRA with differentiable CPU/CUDA retiming #724's backend. Merge bottom-up.
Followed by: Retime NMG rollouts under joint velocity and acceleration limits #714

Description

This PR implements item 1 of #684: measure joint velocity, acceleration and jerk in the motion-generation benchmark, and mark a planner whose reported timing is nominal rather than solved.

The benchmark validated joint positions but never their derivatives, so the dynamic_limits_satisfied_when_applicable term of motion_valid in BENCHMARK_DESIGN.md section 4.2 had no way to evaluate. That matters most for NMG, which integrates action * action_scale per step and reports a fixed nominal dt: 0.2 rad per 0.01 s is 20 rad/s against 2.62 rad/s for the slowest FR3 joint, so its reported derivatives sit on a time axis no arm can execute.

What changed:

Derivatives are recomputed, not read. Velocity, acceleration and jerk come from positions and dt through compute.trajectory.differentiate_positions, rather than from whichever derivatives a planner chose to report. Section 4.2 asks for exactly this, and it keeps a comparison from turning into a comparison of derivative conventions. Repeated three-point differencing damps the higher derivatives, equally for every candidate.
Per-derivative reporting. Peak magnitude, limit utilization and violation for each of the three, plus dynamic_limits_satisfied, which is conjoined into motion_valid and produces a dynamic_limit_violation failure code.
Not-applicable stays distinct from satisfied, end to end. Acceleration and jerk limits have no asset source, so an unset limit yields None from the metric, and _case_macro_rate_optional reports N/A rather than a zero violation rate. Coercing None to False would have reported an unconfigured limit as a clean result.
A position change across a zero interval is an unbounded velocity violation, reported as such instead of letting the differentiator reject the trajectory and fail the whole trial.
PlannerMetadata.native_timing. PlanResult requires dt whenever it carries positions, so a nominal constant cannot simply be withheld. The NMG adapter declares native_timing=False and its trajectory_duration_s aggregates to None, which keeps a nominal duration from being compared against a solved one.
A nominal clock does not decide validity. Peaks, utilizations and violations always describe the timing a planner reported, but dynamic_limits_satisfied — the verdict that feeds motion_valid — additionally requires that timing to be solved. A planner reporting a nominal constant has not claimed an executable duration, so it is neither credited nor failed on a clock it never solved; its diagnostics are still published, which is what makes a nominal and a retimed run of one policy comparable.
Trailing hold padding is trimmed before differentiating. A batched planner pads short rows by repeating the final pose at a zero interval; the first differentiation accepts that hold but steps the velocity across zero elapsed time, and the next stage rejects it outright, so a legitimately padded batch failed evaluation. This is the exact shape _assemble_batched_results produces.
The new metrics are rendered in report.md and summarized over every measured outcome rather than the motion-valid subset. Exceeding a limit can make an outcome invalid, so restricting the population would drop exactly the trajectories with the largest peaks.
Suites carry the published FR3 joint-space limits, 10 rad/s^2 and 5000 rad/s^3 uniformly across all seven joints, with the specification cited inline. Velocity limits continue to resolve from the asset through robot.get_qvel_limits(); the FR3 URDF matches the published table exactly.

Defaults are unchanged: a suite that sets no acceleration or jerk limit reports N/A and its motion_valid is unaffected.

Dependencies: none.

Refs #684

Type of change

Enhancement (non-breaking change which improves an existing functionality)

Measured evidence

Real NMG checkpoint (swa3.onnx, K=5, fingerprint 485fe689…7703b, accepted by the layout validator unmodified) on Franka FR3, smoke suite, 9 free-space cases at W=1/3/5, 3 measured trials each, CUDA.

One unretimed rollout, peak per joint against each joint's own limit:

joint	1	2	3	4	5	6	7
peak \|q̇\| (rad/s)	6.74	3.03	3.09	5.22	1.75	2.27	7.33
FR3 velocity limit	2.62	2.62	2.62	2.62	5.26	4.18	5.26
velocity utilization	2.57	1.16	1.18	1.99	0.33	0.54	1.39
peak \|q̈\| (rad/s²)	329	109	84	269	94	176	692
acceleration utilization	32.9	10.9	8.4	26.9	9.4	17.6	69.2

5 of 7 joints exceed their velocity limit, worst 2.6×; 7 of 7 exceed the acceleration limit, worst 69×. Across the 9 cases the violation rate is 100% on velocity, acceleration and jerk, with utilization 1.21–5.29 and 52–122.

motion_valid stays true for these rows, which is the nominal-timing rule working: a clock the planner never solved publishes diagnostics but does not decide validity. trajectory_duration_s is N/A for the same reason.

The run found two defects this PR had to fix

The velocity check was silently vacuous. The first run reported 0% velocity violations with utilizations around 4e-38. Working back, 13.85 / 4.07e-38 = 3.40e38 — exactly the float32 maximum, identical on every row. Confirmed directly:

qvel_limits = [3.4028234663852886e+38] * 7
qf_limits   = [100000.0] * 7

The simulator does not carry the asset's limits; the URDF declares 2.62–5.26 rad/s and 12–87 Nm. An earlier check that this resolved correctly read the URDF file rather than the runtime value and was wrong. Fixed in 033af89b: a placeholder-magnitude asset limit reads N/A rather than satisfied, protocol.joint_velocity_limit_rad_s lets a suite state what the asset lacks, and the suites carry the published FR3 values.

trajectory_duration_s was set only by the atomic-task scenario, so the duration column was N/A for every free-space row regardless of timing. Also fixed there.

Caveat on the estimator

Peaks are recomputed by repeated three-point differencing, which damps them. Against TOPPRA, whose analytic accelerations are available for comparison, this estimate reads 0.5–3.6% below the analytic value — on six trials TOPPRA's analytic peak exceeded its 10 rad/s² constraint (10.08–10.45) while this estimate read 9.99–10.08. The metric therefore understates an overshoot rather than inventing one. It damps equally for every candidate, so comparisons stay fair, but reported peaks are lower bounds.

One unretimed NMG rollout on Franka FR3: joint velocity and acceleration for all seven joints against the published limits. Velocity limits are per joint, so each level is drawn; the acceleration limit is uniform and its outside is shaded.

Validation

Proportional to the change, which is confined to the benchmark package and its tests:

python -m pytest tests/benchmark/motion_generation/ — 121 passed, 1 skipped.
black . — clean.
python docs/scripts/check_api_docs.py — 2310/2310 exports documented. No public API changed; scripts/benchmark/ is not part of the embodichain package.
context.py affected --base origin/main --explain — no affected topics. BENCHMARK_DESIGN.md is not a registered context topic but is updated here, since section 2.2 listed these checks as missing and section 2.1 described a dt treatment this PR makes explicit.

Three existing tests had to be adjusted rather than changed in meaning. Their shared fixture moves 0.1 rad in 0.025 s, which is 4 rad/s and over a realistic FR3 limit, so the _MetricRobot stub keeps a permissive velocity limit by default and the new dynamic-limit tests set a realistic one explicitly. A test double should not couple unrelated dimensions.

Not included here: nothing bounds or retimes anything yet. This PR only measures. #714 adds retiming and #715 pairs the two arms.

The second commit addresses automated review on this PR; see the comment thread for how each finding was handled.

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.
Public API changes are reflected in the API docs (python docs/scripts/check_api_docs.py), if applicable
I have added tests that prove my fix is effective or that my feature works
Dependencies have been updated, if applicable. No dependency changes were required.

🤖 Generated with Claude Code

greptile-apps · 2026-09-28T14:42:29Z

Confidence Score: 4/5

[High risk] Replaces external TOPPRA package with in-tree trajectory timing implementation.

The PR does not yet appear safe to merge with the unresolved velocity-limit mismatch for the planned retimed benchmark arm.

Findings

Retiming uses different velocity limits ▶

Fix with agent prompt

### Issue 1
scripts/benchmark/motion_generation/metrics/trajectory.py:416-417
When the `nmg_retimed` arm from PR #715 is enabled, this code checks its trajectory against the suite’s FR3 velocity limits. But the retiming adapter added in PR #714 reads the simulator’s placeholder velocity limits instead. As a result, retiming does not enforce the limits used to evaluate the path, and the paired benchmark can report velocity violations for a trajectory presented as retimed under those limits. Retiming and evaluation need to use the same resolved limits.

---

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

Summary

The PR adds benchmark derivative and dynamic-limit reporting, distinguishes NMG’s nominal timing, and expands the branch with in-tree TOPPRA timing, Press planning, and action-contract migration.

Dynamic diagnostics now appear in aggregate and Markdown reports.
TOPPRA gains NumPy and Warp implementations; Press gains a backend-planned path with final-sample validation.
No new actionable issue was established in the changes since the previous review.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  P[Planner positions and timing] --> D[Recompute joint derivatives]
  D --> L[Compare configured dynamic limits]
  L --> R[Report peaks and violations]
  L --> V{Timing solved?}
  V -->|Yes| M[Dynamic-limit verdict for motion validity]
  V -->|No| N[Nominal diagnostics only]

_{Reviews (4) · Last reviewed commit: "fix(bench): do not accept a placeholder ..."}

Follow-up to the same change, from review on #713. - Drop a trailing run of unchanged samples that elapse no time before differentiating. A batched planner pads short rows by repeating the final pose at a zero interval; the first differentiation accepts that hold but steps the velocity across zero elapsed time, and the next stage rejects it, so a legitimately padded batch failed evaluation outright. This is the exact shape `_assemble_batched_results` produces. - Require solved timing before a dynamic result decides `motion_valid`. A planner reporting a nominal constant has not claimed an executable duration, so crediting or failing it on that clock judges an assumption. Peaks, utilizations and violations are still published for those rows, which is what makes a nominal and a retimed run of one policy comparable; `dynamic_limits_satisfied` alone becomes N/A. - Render the new dynamic metrics in `report.md`. They reached `aggregates.json` but no report column, so a reader could not see them. - Aggregate peak jerk alongside peak velocity and acceleration. - Summarize dynamics over every measured outcome instead of the motion-valid subset. Exceeding a limit can make an outcome invalid, so restricting the population dropped exactly the trajectories with the largest peaks and paired a modest peak with a nonzero violation rate. Refs #684 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Yuan-Xinyi · 2026-09-28T15:43:35Z

Thanks — four of the five findings were real and are fixed in 6b322c21.

1. Padded trajectories can fail evaluation (P1) — fixed. Confirmed reproducible: for the padding _assemble_batched_results emits (positions repeated, dt = 0), the first differentiation yields velocity [4, 4, 0, 0] and the second raises A zero time interval cannot change position. Any batched result with unequal-length rows would have failed evaluation. A trailing run of unchanged samples at zero elapsed time is now trimmed first, since those samples are bookkeeping rather than motion. Covered by test_hold_padded_batch_row_still_evaluates.

2. Nominal timing determines motion validity (P1) — fixed. Agreed, and this was the sharper point. dynamic_limits_satisfied is now the verdict and requires solved timing; a planner reporting a nominal constant is neither credited nor failed on a clock it never solved. Peaks, utilizations and per-derivative violations are still published for those rows, so a nominal and a retimed run of the same policy remain directly comparable — which is what #715 uses. Covered by test_nominal_timing_reports_dynamics_without_deciding_validity.

3. Report omits new dynamic metrics (P2) — fixed, independently found while wiring #715. The violation rates, utilizations and peaks are now columns in report.md.

4. Aggregates omit peak jerk (P2) — fixed. max_joint_jerk_rad_s3 is aggregated and rendered alongside the other two peaks.

5. Invalid paths disappear from peaks (P2) — fixed. The dynamics summaries now run over every measured outcome rather than the motion-valid subset, so the trajectories with the largest peaks are no longer the ones excluded from the peak statistics.

greptile-apps · 2026-09-29T01:29:52Z

+    if stated_limit is not None:
+        return resolve_dynamic_limit(stated_limit, asset_limits.shape[-1], reference)


Retiming uses different velocity limits When the nmg_retimed arm from PR #715 is enabled, this code checks its trajectory against the suite’s FR3 velocity limits. But the retiming adapter added in PR #714 reads the simulator’s placeholder velocity limits instead. As a result, retiming does not enforce the limits used to evaluate the path, and the paired benchmark can report velocity violations for a trajectory presented as retimed under those limits. Retiming and evaluation need to use the same resolved limits.

Prompt To Fix With AI

This is a comment left during a code review. Path: scripts/benchmark/motion_generation/metrics/trajectory.py Line: 416-417 Comment: **Retiming uses different velocity limits** When the `nmg_retimed` arm from PR #715 is enabled, this code checks its trajectory against the suite’s FR3 velocity limits. But the retiming adapter added in PR #714 reads the simulator’s placeholder velocity limits instead. As a result, retiming does not enforce the limits used to evaluate the path, and the paired benchmark can report velocity violations for a trajectory presented as retimed under those limits. Retiming and evaluation need to use the same resolved limits. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

The motion-generation benchmark validated joint positions but never the derivatives, so BENCHMARK_DESIGN section 4.2's `dynamic_limits_satisfied_when_applicable` term of `motion_valid` had no way to evaluate. NMG integrates `action * action_scale` per step and reports a fixed nominal `dt`, which puts its derivatives on a time axis no arm can execute: 0.2 rad per 0.01 s is 20 rad/s against 2.62 rad/s for the slowest FR3 joint. - Recompute velocity, acceleration and jerk from `positions` and `dt` through `compute.trajectory.differentiate_positions` rather than reading whichever derivatives a planner reported, so every candidate is measured with one operator. Repeated three-point differencing damps the higher derivatives equally for every candidate. - Report peak magnitude, limit utilization and violation per derivative, plus `dynamic_limits_satisfied`, and conjoin the latter into `motion_valid` with a `dynamic_limit_violation` failure code. - Keep not-applicable distinct from satisfied end to end. Acceleration and jerk limits have no asset source, so an unset limit yields `None` and `_case_macro_rate_optional` reports N/A instead of a zero violation rate. - Treat a position change across a zero interval as an unbounded velocity violation instead of letting the differentiator fail the whole trial. - Add `PlannerMetadata.native_timing`. `PlanResult` requires `dt` whenever it carries positions, so a nominal constant cannot be withheld; the NMG adapter declares `native_timing=False` and its `trajectory_duration_s` aggregates to `None` rather than inviting a comparison against solved timing. - Configure the suites with the published FR3 joint-space limits, 10 rad/s^2 and 5000 rad/s^3 uniformly across all seven joints. Velocity limits continue to resolve from the asset through `robot.get_qvel_limits()`. Refs #684 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Follow-up to the same change, from review on #713. - Drop a trailing run of unchanged samples that elapse no time before differentiating. A batched planner pads short rows by repeating the final pose at a zero interval; the first differentiation accepts that hold but steps the velocity across zero elapsed time, and the next stage rejects it, so a legitimately padded batch failed evaluation outright. This is the exact shape `_assemble_batched_results` produces. - Require solved timing before a dynamic result decides `motion_valid`. A planner reporting a nominal constant has not claimed an executable duration, so crediting or failing it on that clock judges an assumption. Peaks, utilizations and violations are still published for those rows, which is what makes a nominal and a retimed run of one policy comparable; `dynamic_limits_satisfied` alone becomes N/A. - Render the new dynamic metrics in `report.md`. They reached `aggregates.json` but no report column, so a reader could not see them. - Aggregate peak jerk alongside peak velocity and acceleration. - Summarize dynamics over every measured outcome instead of the motion-valid subset. Exceeding a limit can make an outcome invalid, so restricting the population dropped exactly the trajectories with the largest peaks and paired a modest peak with a nonzero violation rate. Refs #684 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Running the suite against a real policy showed the velocity check never fires. `robot.get_qvel_limits()` returns the float32 maximum on all seven Franka joints, not the URDF's 2.62-5.26 rad/s, so every utilization was ~1e-38 and every violation rate zero. Effort limits are a flat 1e5 for the same reason. The earlier check that this resolved correctly read the URDF file rather than the runtime value, and was wrong. - Treat an asset limit that is non-finite or at placeholder magnitude as no limit at all, reported N/A. A velocity check that silently always passes is worse than one reported as unavailable. - Add `protocol.joint_velocity_limit_rad_s` so a suite can state the limit the asset does not carry, and set the published FR3 values in every suite. - Report `trajectory_duration_s` for free-space rows. Only the atomic-task scenario set it, so the duration column was N/A for every free-space row regardless of whether its timing was solved -- which hides the one number that shows what respecting a limit costs. Refs #684 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

yangchen73 · 2026-09-30T18:19:34Z

+    """
+    batched_positions = positions.unsqueeze(0)
+    batched_dt = dt.unsqueeze(0).to(positions.dtype)
+    velocity = differentiate_positions(batched_positions, batched_dt)


The difference method can smooth the velocity and acceleration peaks.

yangchen73 · 2026-09-30T18:20:32Z

    model_revision: str = "N/A"
    separate_prepare: bool = False
    """Whether this backend exposes a distinct lazy preparation phase."""
+    native_timing: bool = True


Default should set to False ?

yangchen73 · 2026-09-30T18:22:51Z

+                for value in values
+            ):
+                raise ValueError(f"{field_name} entries must be finite and > 0.")
+        if self.protocol.dynamic_limit_tolerance < 0.0:


Better to filter NaN or inf?

yangchen73 · 2026-09-30T18:25:15Z

+        return {
+            "max_joint_velocity_rad_s": math.inf,
+            "velocity_utilization": math.inf,
+            "velocity_limit_violation": True,


Logic issue. velocity_limit_violation is meaningless in that case. Better to return N/A.

yangchen73 · 2026-09-30T18:26:01Z

+                    # outcome invalid, so restricting the population here would
+                    # drop exactly the trajectories with the largest peaks and
+                    # pair a modest peak with a nonzero violation rate.
+                    "max_joint_velocity_rad_s": _mean(


_mean() might drops infinite values. consider about that.

Yuan-Xinyi added enhancement New feature or request motion gen Things related to motion generation for robot labels Sep 28, 2026

greptile-apps Bot reviewed Sep 28, 2026

View reviewed changes

Yuan-Xinyi mentioned this pull request Sep 28, 2026

Retime NMG rollouts under joint velocity and acceleration limits #714

Open

6 tasks done

Yuan-Xinyi force-pushed the xinyi/nmg-dynamic-limit-metrics branch from cdf4fe1 to 4dd6607 Compare September 28, 2026 15:36

Yuan-Xinyi mentioned this pull request Sep 28, 2026

Pair a nominal and a retimed NMG arm in the benchmark #715

Open

6 tasks done

greptile-apps Bot reviewed Sep 28, 2026

View reviewed changes

Comment thread scripts/benchmark/motion_generation/reporting.py

greptile-apps Bot reviewed Sep 29, 2026

View reviewed changes

Yuan-Xinyi requested a review from yangchen73 September 29, 2026 04:52

This was referenced Sep 29, 2026

Let NeuralPlanner retime with the differentiable TOPP #725

Closed

Add differentiable time-optimal path parameterization #723

Closed

Yuan-Xinyi and others added 3 commits September 29, 2026 17:15

Yuan-Xinyi force-pushed the xinyi/nmg-dynamic-limit-metrics branch from 033af89 to d65ada7 Compare September 29, 2026 08:30

Yuan-Xinyi changed the base branch from main to feat/differentiable-toppra September 29, 2026 08:30

yangchen73 reviewed Sep 30, 2026

View reviewed changes

Navigation Menu

Measure dynamic limits and mark nominal timing in the NMG benchmark - #713

Measure dynamic limits and mark nominal timing in the NMG benchmark#713
Yuan-Xinyi wants to merge 3 commits into
feat/differentiable-topprafrom
xinyi/nmg-dynamic-limit-metrics

Yuan-Xinyi commented Sep 28, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot commented Sep 28, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Yuan-Xinyi commented Sep 28, 2026

Uh oh!

Uh oh!

greptile-apps Bot Sep 29, 2026

Uh oh!

yangchen73 Sep 30, 2026

Uh oh!

yangchen73 Sep 30, 2026

Uh oh!

yangchen73 Sep 30, 2026

Uh oh!

yangchen73 Sep 30, 2026

Uh oh!

yangchen73 Sep 30, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		if stated_limit is not None:
		return resolve_dynamic_limit(stated_limit, asset_limits.shape[-1], reference)

Conversation

Yuan-Xinyi commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Stack

Description

Type of change

Measured evidence

The run found two defects this PR had to fix

Caveat on the estimator

Validation

Checklist

Uh oh!

greptile-apps Bot commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 4/5

Findings

Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Yuan-Xinyi commented Sep 28, 2026

Uh oh!

Uh oh!

greptile-apps Bot Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

yangchen73 Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

yangchen73 Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

yangchen73 Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

yangchen73 Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

yangchen73 Sep 30, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Yuan-Xinyi commented Sep 28, 2026 •

edited

Loading

greptile-apps Bot commented Sep 28, 2026 •

edited

Loading