feat(eval): traffic-light acceptance gate for planner checkpoints - #391
danielsanchezaran wants to merge 4 commits into
Conversation
Pre-deployment checks for a failure open-loop metrics do not detect: the model plans a standstill on geometry it should drive through, at some values of the ego_shape input. When a corpus mixes platforms whose speeds also differ, the token becomes the cheapest predictor of speed and gets read as a speed prior rather than as geometry; the resulting model can score normally on displacement error while being undeployable. The traffic-light gate is the one to run per checkpoint: grading scenes that span a light cycle with a single threshold marks red frames as stalls AND scores a model that runs reds as healthy, so scenes are grouped by the state their own route lanes carry. Amber is reported, not gated. The remaining tools locate the confound in a corpus, separate token-driven stops from real ones, and identify which input group holds the plan. Every entry point refuses a training milestone that still carries EMA weights, matching the convention in the recovery evals.
danielsanchezaran
left a comment
danielsanchezaran
left a comment
There was a problem hiding this comment.
Blocking review: the implementation is clean mechanically, but the two acceptance gates can still report PASS without establishing their stated invariants. I reproduced both cases below. Otherwise, all 11 focused tests passed on the PR head and GitHub merge ref; Ruff, formatting, compilation, CLI imports, and pre-commit also passed. There are no GitHub checks attached to this head.
|
|
||
| def verdict(spans: dict[str, list[float]], min_green_m: float, max_red_m: float) -> list[str]: | ||
| """Gate failures, empty when the checkpoint passes.""" | ||
| if not spans["green"] and not spans["red"]: |
There was a problem hiding this comment.
P1 — Require both green and red coverage before reporting PASS. This guard only rejects the input when both buckets are empty. I reproduced that a green-only set and a red-only set each make verdict() return no failures, so main() prints PASS even though half of the stated safety contract was never tested. In particular, a green-only set cannot detect a checkpoint that runs red lights. Please require both buckets, or make a deliberately one-sided mode explicit in the CLI and result.
There was a problem hiding this comment.
Fixed in a497bc3 — you're right, and this was the failure the gate exists to catch. verdict() only refused when BOTH buckets were empty, so a green-only set printed PASS with the red half never exercised, and a green-only set is precisely what cannot detect a red-runner. Both halves are now required by default; --allow_one_sided grades what is present, names the untested half in the printed result (PASS (red half NOT tested — not a full acceptance)), and relaxes coverage only, never the thresholds. Four regression tests cover green-only, red-only, the override, and that a red-runner still fails under the override.
|
|
||
| def report_scene(path: str, wheelbases: list[float], sweep: list[float], min_span: float) -> bool: | ||
| """Print one scene's sweep; returns True when it fails the gate.""" | ||
| bad = [s for s in sweep if s < min_span] |
There was a problem hiding this comment.
P2 — Include the sweep spread in the gate verdict. bad only tracks the absolute span floor. For example, report_scene(..., wheelbases=[2.75, 4.76], sweep=[16.0, 40.0], min_span=15.0) prints a 24 m spread followed by pass and returns False. That accepts a strong geometry-dependent speed change even though this command's stated invariant is that planned distance must not depend on the dimension token. Please add a configurable maximum-spread criterion, or rename/document this as a standstill-only gate.
There was a problem hiding this comment.
Valid, and accepted — the docstring promised span independence while the verdict only checked the floor, so a 24 m spread passed. This file is no longer in the PR: the four ego-dimension diagnostics were written to root-cause one specific corpus problem and are being kept with that campaign's assets rather than in shared tooling nothing else exercises. The PR is now the traffic-light gate alone. The spread criterion is being applied to that tool where it lives, so the finding is not lost.
Review P1: verdict() only refused a scene set when BOTH buckets were empty, so a green-only set reported PASS while the red half went untested — and a green-only set is exactly what cannot catch a checkpoint that runs red lights. Both halves are now required; --allow_one_sided grades what is present and names the untested half in the result, which is then not a full acceptance. Four regression tests cover green-only, red-only, the override, and that the override relaxes coverage rather than thresholds. The four ego-dimension diagnostics are dropped from this PR. They were written to root-cause one specific corpus problem and belong with that campaign's assets, not in shared tooling where nothing else exercises them. Review P2 (sweep spread not part of that gate's verdict) is valid and is fixed there rather than here.
danielsanchezaran
left a comment
danielsanchezaran
left a comment
There was a problem hiding this comment.
Follow-up verdict: one blocker remains on the narrowed traffic-light gate. The earlier missing-phase issue is fixed and the four removed diagnostics leave no stale code or README references. All 15 focused tests passed on both the current head and GitHub merge ref; Ruff, formatting, compilation, CLI help, and pre-commit also passed. There are no GitHub checks attached. Separately, please update the PR title and description: they still advertise five diagnostics and 11 tests, while this head contains only the traffic-light gate and 15 tests.
| if not hot.any(): | ||
| return "none" | ||
| present = {_CLASSES.get(int(c)) for c in onehot[hot].argmax(axis=1)} | ||
| for state in ("red", "amber", "green"): |
There was a problem hiding this comment.
P1 — Resolve the controlling signal instead of prioritizing any red segment. This precedence is not valid for the standard route tensor. The existing converter/filter code explicitly notes that route_lanes can include perpendicular red approaches while ego has green (see frame_filters.hpp). I reproduced a route with one valid green segment and one valid red segment being classified as red; measure() will therefore put that ego-green scene in the red bucket and flag a normal plan over 2 m as running the light. Please select the ego-relevant signal using heading/proximity, as the existing detectors do, or reject mixed-state scenes as ambiguous.
There was a problem hiding this comment.
Fixed in 14bd622 — thank you for the pointer to frame_filters.hpp, that comment states the problem better than I could have. The red-wins precedence was my own design choice and it was wrong: a perpendicular red would put an ego-green scene in the red bucket, where a normal plan is then reported as running a light the ego never faced.
I did not port the heading/proximity resolution. That geometry already exists in detect_red_light_run, and a second Python copy with nothing cross-checking it is how the two drift apart — so I took your other option. A route whose signalled lanes disagree is now classified ambiguous and excluded from the verdict, alongside scenes with no signalled lane; both counts are printed so the excluded set is visible rather than silent. Fewer graded scenes, no invented answer.
The test that asserted the old precedence encoded the bug, so it now asserts exclusion and that agreeing lanes still classify normally. Bucketing reads only route_lanes, so this is model-independent: on the 30-scene reference set it yields 0 ambiguous and the split is unchanged at 23/5/2, meaning the previously reported acceptance numbers are unaffected — the defect would have bitten at multi-approach intersections, which that set does not contain.
PR title and description updated to match this head (traffic-light gate only, 15 tests).
Review P1: route_lanes is not one signal. At an intersection it can carry a perpendicular approach that is correctly red while the ego's own approach is green, so red-wins precedence put ordinary green scenes in the red bucket, where a normal plan is then reported as running a light the ego never faced. Resolving which signal governs the ego needs heading alignment and stop-line proximity, which already exists in the C++ frame filters (detect_red_light_run). A second, uncross-checked Python copy of that geometry is how the two drift apart, so this takes the other option offered in review: a route whose signalled lanes disagree is classified ambiguous and excluded from the verdict, alongside scenes with no signalled lane. Both counts are printed so the excluded set is visible rather than silent. The test that asserted the old precedence encoded the bug; it now asserts exclusion, and that agreeing lanes still classify normally. Bucketing reads only route_lanes, so this is model-independent: on the 30-scene reference set it yields 0 ambiguous and the split is unchanged at 23 green / 5 amber / 2 red.
danielsanchezaran
changed the title
danielsanchezaran
left a comment
danielsanchezaran
left a comment
There was a problem hiding this comment.
Follow-up approval on 14bd6228d: both previously reported blockers are resolved. The gate now requires green and red coverage by default, and mixed route-lane signal states are classified as ambiguous instead of assigning an unrelated red approach to the ego. The removed campaign-specific diagnostics left no stale shared-tool references, and the PR title/description now match the narrowed traffic-light-gate scope.
I found no remaining correctness issues. All 15 focused tests passed on both this exact head and the GitHub merge ref; Ruff, formatting, compilation, CLI help, and pre-commit also passed. There are no GitHub checks attached to this head. Looks good to merge from code review.
Posting as a comment because GitHub does not allow approving your own PR.
A mean is not enough to read this gate. Two cases it hides: A class whose scenes all return the same span is one situation sampled many times, not many independent samples -- its n overstates the evidence. And a class that splits into 'drives' and 'stands still' can average out above the threshold while a chunk of its scenes are collapsed: 18 scenes at 20 m and 5 at 1 m means 15.87 m, which clears a 15 m floor with 5 failures inside it. Both are visible in min/median/max and invisible in the mean, so print all four, and flag a class whose spread is under a centimetre. Compared with a tolerance rather than for equality: repeated measurements of one situation still differ in the last decimals, so an equality test never fires on real data. Verdicts are unchanged -- the gate still grades on the mean, so existing results reproduce exactly. This only makes the evidence behind them legible.
1 participant
A pre-deployment check on a planner checkpoint: does it plan to go on green and hold on red? Seconds of compute, no closed-loop simulation.
Why this is not just a plan-length threshold
Open-loop trajectory error detects neither failure. A model can hold a standstill plan at signalised geometry and still score normally on displacement metrics; a model that drives through stop lines scores normally too, because the recorded future it is compared against is a car that stopped for perfectly ordinary reasons.
Grading scenes taken across a light cycle with one span threshold is actively wrong: frames where standing still is correct get marked as stalls, and in the same breath a model that runs reds is scored as healthy, since running a red produces exactly the long confident plan the threshold rewards. So scenes are grouped by the traffic-light state their route carries, and each group is held to its own standard — green must commit to go, red must hold, amber is reported but never graded (easing and proceeding are both defensible).
What makes a result trustworthy
--allow_one_sidedoverrides and names the untested half in the result, which is then not a full acceptance.route_lanescan carry a perpendicular approach that is correctly red while the ego has green. Resolving which signal governs the ego needs heading alignment and stop-line proximity, which already exists in the C++ frame filters (detect_red_light_run); rather than keep a second uncross-checked copy of that geometry, a route whose signalled lanes disagree is classifiedambiguousand excluded from the verdict. Excluded counts are printed, not silent.Scope
This PR is the traffic-light gate alone. Four ego-dimension diagnostics were in an earlier revision and have been removed: they were written to root-cause one specific corpus problem, and belong with that campaign's assets rather than in shared tooling nothing else exercises. The review finding against one of them (sweep spread printed but not gated) was valid and has been applied where that tool now lives.
Verification