the stager stopped doing the same work three times - #49
Conversation
_windowRmssd and _windowRemFeatures each hand-copied _cleanBeatsInWindow, which is exactly what that function's docstring says can't happen. folded both back into it; the rem one passes halfWinMs: 90s because its window is deliberately different (spanSec, the 240-point ls grid and the <16 abstain gate are all specified against ±90s, not ±2.5min) — noted that in the doc so nobody "simplifies" it later. also fixed the docstring rationale: it claimed the beats had to match so the z-scores stayed comparable, but rmssd isn't z-scored any more. real-night gate unchanged: wake=52 rem=300 nrem=716 deep=137, same confidence to the last digit.
the rolling hr baseline took median(win) and percentile(win, 25) of the same 361-epoch window, and each of those sorted a fresh copy — 25 lines above the comment bragging about saving sorts. sort win in place once (it's built right there) and take both off it. also hoisted the median(sleepHr) that ran twice in the night-observation record. percentileSorted keeps the double? / null-on-empty contract. the two private _percentileSorted copies in advanced_stager and load_trimp return 0 on empty — not promoting that, a 0.0 bpm hr floor recorded as a measurement is a bug we've already had. deduping those two is a separate job. real-night gate unchanged: wake=52 rem=300 nrem=716 deep=137.
the per-epoch window gather walked all ~40k beats of the night to find the few hundred inside ±2.5 min, once per epoch per feature. lower-bound in, break at the far edge. the bound has to be a TRUE lower bound. rr_ts_ms is rec_ts * 1000 so beats tie on the second boundary, and "first index strictly greater than lo" drops the tied beats sitting on the edge — i tried it: deep goes 137 -> 132 epochs on the real night, silently. test pins ties at both edges and fails on that exact mutation. no cursor threaded through the epoch loop on purpose — centreMs comes from accel, not rrTsMs, so a cross-epoch cursor would assume an ordering the caller was never asked for. cardioStager on the real night: ~950ms -> ~805ms, so about 15%. not more — lomb-scargle's trig is the rest of it and this doesn't touch it. counts unchanged: wake=52 rem=300 nrem=716 deep=137.
|
Warning Review limit reached
Next review available in: 51 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review stacked on #48, targets that branch. same deal — this one is meant to be bit-identical on output, so any behaviour change you spot is a bug not a tradeoff. |
|
|
1 participant
Stacked on #48. Output must be bit-identical — there is no
kAlgoVersionbump in this PR, and if one becomes necessary the change is wrong.An optimizer pass went through the tree arguing for deletion; every claim it made was then handed to a separate agent that did not know who made it and was told to assume it was wrong. These three survived that. One claim elsewhere was refuted outright — structurally accurate, but its proposal was slower than the code it replaced.
the safety argument
The real-night gate asserts wide bands (
wakeMin < 35,remMin > 100) and would not have noticed a small move, so the numbers were pinned directly instead:Identical after all three commits, confidence to the last digit.
what changed
_windowRmssdand_windowRemFeatureseach hand-copied_cleanBeatsInWindow, which is the function whose own docstring promises the invariant they were breaking. Folded back in;_cleanBeatsInWindowtakeshalfWinMsnow. REM keeps its own 90 s window and the docstring says why, so nobody "simplifies" that later — handing it the ±150 s list re-spaces the Lomb–Scargle grid, defeats thebeats.length < 16abstain gate, and re-opens a previously fixed ANR. The stale rationale in that docstring is corrected too: rmssd is not z-scored any more.The rolling HR baseline sorted the same 361-epoch window twice, about 25 lines below a comment congratulating itself on not doing that.
percentileSortedsplit out ofpercentile, window sorted in place once, duplicatedmedian(sleepHr)hoisted.percentileSortedkeeps the nullable contract — there are two_percentileSortedcopies in this repo that return0on empty and they stay where they are; a 0-on-empty percentile beside a null-on-empty one is how a 0.0 bpm floor gets recorded as a measured baseline.Two window scans are binary searches now. True lower bound, per call, no cursor threaded through the epoch loop —
centreMscomes fromaccel, notrrTsMs, so a cross-epoch cursor would assume monotonicity nothing promises.The tie trap is pinned by a test, not a comment.
rr_ts_msisrec_ts * 1000, so beat timestamps tie. Writing the search the natural way — first index strictly greater — fails the new test 7 → 4 and moves the real night:deep 137 → 132,light 579 → 584. That mutation is now caught.measured
cardioStageron the full real night: ~950 ms → ~805 ms, about 15% (medians of 6 and 5 runs). The original claim implied ~200x; Lomb–Scargle's trig dominates and is untouched. The commit messages carry the measured figure, not the claimed one.580 tests, analyze clean.
deliberately not done
Deduping the two
_percentileSortedcopies. Hoisting the gather out of_windowRmssd/_windowSdnnto the epoch loop — it would halve that work again but changes two signatures. A runtime sorted-ness guard onrrTsMs, which would cost what the search saves; the requirement is documented instead.