fix: a reference controller scan clock counted the period, not the time - #1
Closed
malgaroshy-maker wants to merge 1 commit into
Closed
malgaroshy-maker wants to merge 1 commit into
malgaroshy-maker wants to merge 1 commit into
Conversation
…time Nine graded tests failed on Linux CI and passed on Windows. They read as a flaky grader. They were a controller whose clock was wrong. `run_scan` called `body(period)` -- handing every reference controller the NOMINAL scan period as its dt -- and then slept that long. On an idle machine an iteration really does take about `period`, so the controller and the plant agree. On a busy one each iteration takes period plus the body plus whatever the event loop was doing, and the plant advances by that whole amount because it runs on its own wall-clock accumulator. The controller counted only `period`, so it under-counted elapsed time by an amount that depends purely on machine load. That is AGENTS.md gotcha 3 arriving somewhere new: "stepping once per sleep(tick_ms) runs the sim slow... both engines accumulate real elapsed time". The engines learned it. The reference controllers did not. It showed up first in the batch-dosing stopwatch, which computes a cut-off in seconds: 23.8 L against a 22 L pot on Linux, 22.0 L here, from the same code. `dt` is now real elapsed time. Nine failures became one. The last one was a claim the controller never made. The test asserted the stopwatch's FIRST batch lands inside tolerance. A stopwatch with no taper cuts off at a scan boundary, so it overshoots by up to one scan's delivery -- 2 L/s at the rated flow -- and how coarse the scans get is a fact about the machine, not the program. Worse, the overshoot is the thing the scene teaches, so pinning it pins the lesson to a machine. That assertion is gone and the reasoning is in its place, while the lesson it was standing near got stronger: batch two delivering about half now has both bounds where it had one. Verified: the batch-dosing case passes on Linux where it failed; full Linux grader suite re-running to confirm 51/51 (it was 50/51 before this). Not fixed here, and worth its own change: CI's Python step is about thirty minutes because these graded tests are nearly all of it. They should run as their own job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
Opened by mistake — closing. The fix itself is unaffected and will land separately. |
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
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.
What this changes
Every reference controller in
tools/grade.pynow measures its scan with realelapsed time instead of assuming its nominal period. One test assertion is
removed, and the reasoning is left in its place.
Why
Nine graded tests failed on Linux CI and passed on Windows. They read as a
flaky grader. They were a controller whose clock was wrong.
run_scancalledbody(period)— handing every reference controller thenominal scan period as its
dt— and then slept that long. On an idlemachine an iteration really does take about
period, so the controller and theplant agree. On a busy one each iteration takes
periodplus the body pluswhatever the event loop was doing, and the plant advances by that whole amount,
because it runs on its own wall-clock accumulator. The controller counted only
period, so it under-counted elapsed time by an amount that depends purely onmachine load.
That is AGENTS.md gotcha 3 arriving somewhere new — "stepping once per
sleep(tick_ms)runs the sim slow... both engines accumulate real elapsedtime". The engines learned it; the reference controllers did not.
It surfaced in the batch-dosing stopwatch, which computes a cut-off in seconds:
23.8 L against a 22 L pot on Linux, 22.0 L on Windows, from the same code.
Two things I tried first that did not work, since this project's log keeps
those:
wall-clock to the plant's clock (already on
master). That change is righton its own merits — a grader whose mark moves because the marking machine
was busy is broken regardless — but it fixed nothing here, and I could not
reproduce the failure with it either way. I was fixing the run's clock
while the controller's clock was wrong.
dttook nine failures to one. The survivor was not a bug at all.That last failure was a claim the controller never made. The test asserted the
stopwatch's first batch lands inside tolerance. A stopwatch with no taper
cuts off at a scan boundary, so it overshoots by up to one scan's worth of
delivery — 2 L/s at the rated flow — and how coarse the scans get is a fact
about the machine, not about the program. The overshoot is also the thing the
scene teaches, so pinning it pins the lesson to a machine. That assertion is
gone; the lesson beside it got stronger, gaining a lower bound where it had
only an upper one.
What you ran
Reproduced on Linux (WSL2, Ubuntu 26.04), which is where CI runs and where the
failure lives — it has never failed on Windows.
Before, single test:
The grader's own report named the cause:
After the
dtfix, same test, same machine:Full Linux grader suite, after the
dtfix and before the assertion change —nine failures down to one:
Windows, bus and parity suites, unaffected by this change:
The full Linux grader suite is re-running to confirm 51/51 with both changes in.
I will post that output on this PR when it lands rather than claim it now.
Checks
python -m pytest -qpassespython tools/test_plan.py— section B is the suite above; the full plantakes ~45 min with the grader in it and is running
dotnet buildinengine/is clean at zero warnings — untouched bythis change, verified on
masterata8e5b41If you added a test:
this change runs the other direction: I watched a failing test, found the
product bug behind it, and removed one assertion that was measuring the
machine rather than the code. Both are documented above.
Docs
reasoning is now a comment at the site, so its absence is legible.
Related
Follows the HP-33 write-gate fix (
09f1b44) and the timeout-handler fix(
a8e5b41), both of which were needed before CI could report this failure atall — the handler meant to print
TIMEOUTwas raisingTypeErrorinstead.Not fixed here, and worth its own change: CI's Python step is about thirty
minutes because these graded tests are nearly all of it. They should run as
their own job so a fast signal stays fast.