Repository navigation
sleep: wire the HR-only spine, on a percentile anchor rather than the median - #1806
Merged
Merged
Conversation
… median Wires hrOnlySessions into the day scan for a strap with no motion, and fixes a real flaw in the merged spine that writing the tests exposed. THE ANCHOR. hrOnlySleepRuns took its band from hrBaseline, the window MEDIAN, on the reasoning that reusing the motion path's definition was parity. That was wrong, and wrong by arithmetic rather than by tuning: a median splits the samples in half by definition, so a band of median * 1.05 admits strictly more than half of ANY window. Measured on a realistic 24 h - 16 h awake at 74-96, 8 h night at 59-70 - it called 14.4 hours sleep against a truth of 8. hrSleepBandMult is correct where it was designed to be used, as a CONFIRMATION gate on a run gravity stillness already found, where being permissive is deliberate. A confirmation threshold and a detection threshold are not the same instrument. Now anchored on hrOnlyBaseline, the tenth percentile by nearest rank, which sits in the night's trough and cannot admit half a window however the day is shaped. Chosen from a sweep over anchor x multiplier against windows with known truth, taking the conservative corner because the two failure directions are not symmetric: over-detection puts a wrong Rest on screen, under-detection leaves "No data", which is the state this improves on. p10 x 1.05 measures 8.1 h against a truth of 8 on a field-shaped day and under-reads a 54 h two-night window rather than over-reading it. MERGE THE RUNS. A run boundary is a threshold crossing and a sleeping heart rate oscillates across the band all night, so the spine returned the night's minutes shredded into sub-mergeMin fragments, every one of which then failed the minimum-duration gate - 8 h of detected sleep yielding zero sessions. mergePeriods first, exactly as the motion path does. DISPLAY-ONLY, ENFORCED STRUCTURALLY. An HR-only night carries NULL restingHR and avgHRV, and the AnalyticsEngine enrichment that would refill them now skips it. Those two values are what Charge and the baselines fold in, and a baseline is the one thing a false positive cannot be unwound from. Withholding them is structural; a downstream filter would be one forgotten call site from failing open. The Swift enrichment also now names hrOnly when it rebuilds the struct - unlike Kotlin's copy, that rebuild drops a new field by default. A 4.0 IS UNTOUCHED. The fallback sits behind grav.size < 2 and only when the device persisted no hypnogram of its own, so a strap that banks motion keeps the motion spine - a working detector must not be replaced by a weaker one. Pinned by a source-reading test. Two comments corrected that this change falsified: the no-motion diagnostic no longer claims the stager has no HR-only fallback, and the Apple day-cache no longer justifies itself on "a WHOOP always streams gravity". 18 Kotlin tests, 0 failures. Swift twins compile; the package test binary cannot link GRDB's sqlite3 on Linux, so CI runs them for real. Refs #1801.
Re-review, and the finding is the exact failure this PR claimed to have designed out.
The display-only guarantee rested on an HR-only session carrying NULL restingHR/avgHRV. That only
protects an aggregate that READS those fields, and two of the day's physiological aggregates do not:
- the deep-window HRV pool re-derives RMSSD from `rr` over each session's own stages;
- the SDNN index filters `rr` to the matched session windows.
An HR-only night HAS stages, so both would have folded one in — and from there into Charge and the
HRV baseline, which is the one thing a false positive cannot be unwound from. deepHrvWindow is a user
setting (UnitPrefs.hrvWindow == DEEP_SLEEP), so that path is reachable rather than theoretical.
Fixed by naming the set once — physiologySessions = matched.filter { !it.hrOnly } — and routing all
four aggregates through it, rather than adding a filter at each site. The scattered version is what
produced this bug: I wrote "a downstream filter would be one forgotten call site away from failing
open" and then forgot two.
`matched` keeps its legitimate uses in analyzeDay — duration, stage totals, Rest, the onset — which
is why the guard checks the physiological region rather than banning the name.
Guarded by a source-reading test, deliberately rather than a behavioural one: the failure this
protects against is a FUTURE aggregate reaching for `matched` out of habit, and a behavioural test on
one path would not catch that. 20 Kotlin tests, 0 failures.
…e percentile twin Re-review, two findings. WakeMotionRefinement rebuilds a SleepSession field by field on Apple, so it dropped hrOnly by default; the Kotlin twin is a `copy` and preserved it for free. Refinement only ever sees motion-detected sessions today, so an HR-only night never reaches it — but the two platforms would have disagreed silently the moment it did, and a flag that decides whether a night can touch the baselines is the worst one to lose quietly. This is the second time in this PR that Swift's explicit init dropped the field where Kotlin's copy kept it; both sites now name it. The percentile index is now pinned against the Swift twin's own stdout across the sizes where a nearest-rank rule can disagree: n below 1/p, and either side of an exact boundary (1, 2, 3, 7, 10, 11, 99, 100, 101, 1000). Both platforms truncate toward zero, so they agree — but that is now asserted rather than reasoned, since a rounding rule is exactly the kind of thing that reads identical and diverges. 21 Kotlin tests, 0 failures.
app-build caught what nothing else could. Strand/Data/IntelligenceEngine.swift is the day scan and lives OUTSIDE the StrandAnalytics package, so an internal entry point is invisible to it: Strand/Data/IntelligenceEngine.swift:1182:39: error: 'hrOnlySessions' is inaccessible due to 'internal' protection level Both app targets failed on it while every default CI job stayed green, because none of them builds the app target — `swift build` compiles the package, where internal resolves fine, and that is what I had been checking locally. Only the entry point is promoted. The spine, the anchor and hrOnlyBaseline stay internal: the tests reach them with @testable, and nothing outside this package should be assembling its own spine.
ryanbr
added a commit
that referenced
this pull request
Sep 2, 2026
Staging bump, so all three numbers move together: MARKETING_VERSION and versionName to 11.1.0, and the build numbers past the staging high-water — versionCode 411 -> 412 and CURRENT_PROJECT_VERSION 292 -> 293. 411 was taken by the 11.0.0 release, so 412 is the first free code a side-installed staging build can carry without the installer refusing it as a downgrade. fork-testing-build.yml reads MARKETING_VERSION out of project.yml for the asset names and the release title, so the testing artefacts will read v11.1.0. Carries the HR-only sleep spine (#1805) and its wiring (#1806): a strap that streams heart rate but banks no motion can now stage a night from HR alone, marked lower-confidence and kept out of Charge and the baselines. No schema change, so no migration.
ryanbr
added a commit
that referenced
this pull request
Sep 2, 2026
Perf audit of this PR, second finding. The epoch count built a set over EVERY sample -
mapTo(HashSet()) on Kotlin, Set(hrS.map{}) on Swift, the latter allocating a full intermediate array
as well - to arrive at a few thousand distinct keys. That is ~160k boxed values per scored day across
the 21-day rescore, and it is precisely the allocation shape #707 names as the heap-churn source
under the OOM.
hrS is already sorted by timestamp, so the bucket key is non-decreasing and a running comparison
counts them with no allocation at all. Pinned by a test, because the zero-allocation version trades
the set for that ordering assumption: unsorted input would silently over-count rather than fail.
The rest of the audit came back clean, and the parts that look expensive are not this PR's. The bpm
boxing in the percentile sort and the epoch HashMap inside hrOnlySleepRuns both predate it - they
arrived with the spine in #1805/#1806 - and this change does not add to either. Worth a separate look
if the spine ever runs on every day rather than only motionless ones; not worth widening a
diagnostic PR to chase.
26 HR-only tests, 0 failures; swift builds; doc_comment_lint clean.
ryanbr
added a commit
that referenced
this pull request
Sep 2, 2026
…fallback Re-review. Removing the owner exclusion does not only unblock the 5/MG it was written for - it makes the branch reachable for ANY owner, including a WHOOP 4.0 day that banked nothing at all. I had told the maintainer a 4.0 was untouched "because grav.size < 2 is false", which is true of a normal 4.0 day and not true of an empty one. Low risk, and not zero: such a day has too little of anything to clear minSleepMin, but "too little" is not "none", and the night it could produce is display-only and marked hrOnly like every other. The comment now says so at the branch rather than leaving the next reader to work out that the gate widened. Also verified this pass, and sound: the guard test splits the expression on "else ->", and there is exactly one of those, one importedDeviceId, none after the split. The middle branch returning emptyList() for an import-namespace day with stored sessions preserves what both #804 and pre-#1806 did - that path has always yielded nothing there, so nothing moved. And a normal WHOOP day never reaches the stored lookup at all, since grav.size >= 2 short-circuits it, so the added query lands only on days that were already scoring blank.
ryanbr
added a commit
that referenced
this pull request
Sep 2, 2026
…eep WHOOP out (#1813) The spine had never once run. The gate line from #1812 said so on the first build that carried it: [sleep] hr-only gate attempted=false reason=imported-owner grav=0 stored=0 on a day holding 165,980 HR rows - skipped before any heart rate was looked at. Neither of the two failures the funnel was built to separate; it never got that far. #1806 took its condition verbatim from #804 Fix A, whose comment says it exists "so WHOOP straps and the my-whoop import namespace are untouched". #804 is about Oura RINGS and deliberately excludes WHOOP. The HR-only fallback is FOR a WHOOP strap, so it inherited a condition whose entire purpose was to exclude the case it was built for. resolveDayOwner returns importedDeviceId whenever the owner source is absent or the candidates collapse to it, which on a live 5/MG install is every day - so the p10 anchor tuned in #1806 was never reached either. Two owner concepts disagree, which is why older logs could not show this: the [universal] dayOwner line reports the strap id while resolveDayOwner returns the imported one. Split by question rather than by gate. #804's stored-hypnogram branch keeps the owner exclusion. The heart-rate fallback drops it. Between them a new branch leaves any day alone that already has stored sessions, whoever owns it, because inferring a night from heart rate when the device recorded a real one replaces better evidence with worse. The stored lookup now runs for every no-motion day, so "nothing else knows about this night" is checked rather than assumed - one indexed read, and only on days that were already scoring blank, since a normal WHOOP day short-circuits at grav.size >= 2. Re-review recorded what dropping the exclusion widens: the branch is now reachable for ANY owner, including a WHOOP 4.0 day that banked nothing at all. A normal 4.0 day never reaches the gate, and an empty one has too little to clear minSleepMin - but "too little" is not "none", and I had described a 4.0 as untouched more broadly than was true. Both platforms. 26 HR-only tests, 0 failures. The Apple half is app-target Swift that no default CI job builds; the testing build's iOS and macOS legs are what compile it. Refs #1801.
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.
Refs #1801. Wires
hrOnlySessionsinto the day scan for a strap with no motion — and fixes a realflaw in the spine merged in #1805 that writing the tests exposed.
The anchor was wrong, by arithmetic
hrOnlySleepRunstook its band fromhrBaseline, the window median, on the reasoning thatreusing the motion path's definition was parity. That was wrong, and not as a matter of tuning:
Measured on a realistic 24 h — 16 h awake at 74–96, 8 h night at 59–70 — it called 14.4 hours
sleep against a truth of 8.
hrSleepBandMultis correct where it was designed to be used: a confirmation gate on a rungravity stillness already found, where permissiveness is deliberate. A confirmation threshold and a
detection threshold are not the same instrument, and #1805's doc argued they were.
Now anchored on
hrOnlyBaseline— the tenth percentile, nearest rank — which sits in thenight's trough and cannot admit half a window however the day is shaped.
Why p10 × 1.05
Swept anchor × multiplier against windows with known truth, taking the conservative corner
because the failure directions are not symmetric: over-detection puts a wrong Rest on screen,
under-detection leaves "No data", which is the state this feature improves on.
hrOnlyBandMultis a separate constant fromhrSleepBandMult— equal today, but a detector'stolerance and a confirmation gate's have no business moving together.
Merge the runs, or the night never survives
A run boundary is a threshold crossing, and a sleeping heart rate oscillates across the band all
night. Without
mergePeriodsthe spine returned the night's minutes correctly but shredded intosub-
mergeMinfragments, every one of which then failed the minimum-duration gate — 8 h ofdetected sleep yielding zero sessions. The motion path calls
mergePeriodsfor exactly thisreason; now so does this one.
Display-only, enforced structurally
An HR-only night carries null
restingHRandavgHRV, and theAnalyticsEngineenrichmentthat would refill them now skips it. Those two values are what Charge and the baselines fold in, and
a baseline is the one thing a false positive cannot be unwound from. Withholding them is structural;
a downstream filter would be one forgotten call site from failing open.
The Swift enrichment also now names
hrOnlywhen it rebuilds the struct — unlike Kotlin'scopy,that rebuild drops a new field by default.
A WHOOP 4.0 is untouched
Reported working, and it must stay that way: the fallback sits behind
grav.size < 2and onlywhen the device persisted no hypnogram of its own, so a strap that banks motion keeps the motion
spine. A working detector must not be replaced by a weaker one. Pinned by a source-reading test.
Two comments this change falsified, corrected
no-motiondiagnostic no longer claims the stager has no HR-only fallback;the HR-only
providedSleepderives from the very HR the cache key already fingerprints).Verification
counts, not a zero-match filter.
sqlite3on Linux, so CI runsthem for real.
compileFullDebugKotlin,swift build,doc_comment_lint,i18n_audit --ciall clean.Strand/Data/IntelligenceEngine.swiftis app-target Swift that no default CI job builds —wants an app-build dispatch before merge.
known truth. They establish how much of a window the detector claims; they cannot establish that
the staging inside a claimed night is accurate.