Sleep stages: recognise both spellings of wake in segment comparisons (#979) - #1011
Merged
Conversation
…#979) Stored hypnograms carry both 'wake' and 'awake'. Five segment comparisons on EACH platform recognised only one, and they are the same five sites — the reimplementation reproduced the bug faithfully: SleepStager 1075/2193 <-> SleepStager.kt 1157/2378 WakeMotionRefinement 188 <-> WakeMotionRefinement.kt 223 WearableImporter 134 <-> WearableExportImporter.kt 788 XiaomiImporter 136 <-> XiaomiBandImporter.kt 335 The damaging shape is `stage != "wake"` used to mean asleep: an imported 'awake' segment falls through it and is counted as SLEEP, inflating the sleep efficiency figure. The mirror shape `stage == "wake"` under-counts wake time, and made #987's wake refinement skip those segments entirely. Six other sites already defended with `case "wake", "awake"`, which is what makes this a missing shared rule rather than a missing idea. Two vocabularies exist by design and neither moves here: segment strings canonicalise to 'wake', minutes-dictionary keys to 'awake'. The bug is the dictionary vocabulary reaching a segment comparison, which happens because imports do not pass through SleepStagerV2 — Oura's phase table is [deep, light, rem, awake]. So this is a PREDICATE, not a canonicaliser: it fixes the comparisons without rewriting any stored string, and no persisted hypnogram changes meaning. SleepStagerV2's own "awake" tests are deliberately untouched; that is its internal model vocabulary, renamed to 'wake' on output. Android's UI canonicalStage() now folds through the shared predicate so the alias rule has one definition. It still returns 'awake', because that is the key its colour table and the minutes dictionaries use. 6 twin tests per platform, same cases in the same order.
The six predicate tests pass whether or not the five call sites were actually changed - they exercise the rule, not its users. Nothing in the suite noticed a reverted site, which for a latent bug is the only way a regression would ever surface. Each platform now also drives a real caller: SleepStager.hypnogramMetrics over the same night as the existing AASM test, with the WASO segment spelled 'awake'. tst comes from a positive list (light/deep/rem) so it is immune either way at 1080 s; WASO and the disturbance count are not, and read 0/0 before the fix. Verified the two implementations agree before asserting: onset, sptEnd and the clip arithmetic are identical in Swift and Kotlin, so both expect waso 60 and disturbances 1.
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.
From the "also recorded" note in #988, filed separately as it asked.
Stored hypnograms carry both
wakeandawake. Five segment comparisons on each platform recognised only one of them — and they are the same five sites, because the Kotlin reimplementation reproduced the bug faithfully:SleepStager.swift:1075,:2193SleepStager.kt:1157,:2378WakeMotionRefinement.swift:188WakeMotionRefinement.kt:223WearableImporter.swift:134WearableExportImporter.kt:788XiaomiImporter.swift:136XiaomiBandImporter.kt:335What goes wrong
The damaging shape is
stage != "wake"used to mean asleep. An imported"awake"segment falls straight through it and is counted as sleep, inflating the efficiency figure. The mirror shape,stage == "wake", under-counts wake time — and made #987's wake refinement skip those segments entirely, so the transition rule that shipped in 9.3.0 was silently not applying to imported nights that use the other spelling.Six other sites already defend with
case "wake", "awake", andRepository.swift:1339/WhoopRepository.kt:733get it exactly right. That is what makes this a missing shared rule rather than a missing idea.Why a predicate and not a canonicaliser
There are two vocabularies, by design, and neither moves here:
stagestrings canonicalise to"wake"—SleepStagerV2models its own states as"awake"internally and renames on the way out, saying so in its stage-mapping comment."awake"(SleepStageTotals,SleepWindowReclip).The bug is the dictionary vocabulary reaching a segment comparison, which happens because imports never pass through
SleepStagerV2— Oura's phase table is["deep","light","rem","awake"], and generic wearable JSON carries whatever the source app wrote.So
SleepStageVocabulary.isWake(_:)is a predicate. It fixes the comparisons without rewriting a single stored string, so no persisted hypnogram changes meaning.SleepStagerV2's own== "awake"tests are deliberately untouched — that is its internal model vocabulary, and changing it would move stored output.Parity
Twin helpers, twin tests, same cases in the same order. Android's UI
canonicalStage()now folds through the shared predicate so the alias rule has one definition; it still returns"awake", because that is the key its colour table and the minutes dictionaries use.Worth recording: the two platforms canonicalise in opposite directions — Android's UI folds
wake -> awake, Swift'sSleepStagerV2foldsawake -> wake. Harmless today because each is internal, but they are each other's alias, which matters if a canonical stage string ever crosses the.noopbakboundary. Not changed here; that decision deserves its own PR.Verification
OK (6 tests). The Swift twin runs inswift-packagesCI. Cases: both spellings, sleep stages excluded, casing/whitespace folded, unknown/empty not wake, plus the two regressions in the shapes the callers actually use — anawake+deepnight counting onlydeepas asleep, and a wake total including both spellings.src/main/javacompile, 2517 errors on this branch and 2517 on main, error sets byte-identical. No new errors.swiftc -parseclean on every changed Swift file and the new test.Tools/doc_comment_lint.pyclean.Correction: this is latent, not active — no release note needed
My first draft of this section warned that sleep numbers would move for imported nights, "Oura in particular". I went looking for the affected nights and could not find any, so that warning was wrong and is withdrawn.
No producer on either platform writes
"awake"into a stored segment. NOOP's own scorers emit"wake"(SleepStagerV2renames its internalawakeon output; V1 writeswake).XiaomiImportermaps.awake/.awakeInBed/.unknownto"wake".FitbitExportParserfolds"wake","awake"and"restless"to"wake". Oura's"awake"appears in a doc comment describing the upstream wire format, not in anything stored. The only"awake"segment in the tree is a test fixture — which is itself evidence that someone expected it reachable.So no stored night changes today, and there is nothing to put under "scores that change".
That does not make the five sites correct. They are wrong, and the spelling arrives the moment anything bypasses the normalising importers — a new importer that passes its source vocabulary through, a hand-edited hypnogram, or #746's row-copy import of a backup from another fork, which copies segments verbatim from a tree whose vocabulary we do not control. This closes the hole before that lands rather than after.
The honest summary: correctness hardening with a real failure mode and, as of today, no behavioural blast radius.
app-build.ymlis disabled, so nothing compiles the twoStrand/Dataimporters — those edits are parse-checked only. The analytics half and both test suites are covered by CI.Re-review: the tests did not test the fix
The six predicate tests pass whether or not the five call sites were actually changed — they exercise the rule, not its users. For a latent bug that matters more than usual: nothing else would ever notice a reverted site, because no production data reaches it today.
Each platform now also drives a real caller.
SleepStager.hypnogramMetricsover the same night as the existing AASM test, with the WASO segment spelledawake:tstis computed from a positive list (light || deep || rem), so it is immune either way and pins at 1080 s — that is also why my sweep did not need to touch it.wasoSanddisturbancesare not immune. They read 60 / 1 with the fix and 0 / 0 without it, so a reverted call site fails the suite.I checked the two implementations agree before writing the expectations rather than assuming parity: onset,
sptEndand the clip arithmetic are identical in Swift and Kotlin.Also confirmed this pass:
SleepStager.swift:2193feeds WASO and the disturbance count, not an internal total — so the mirror shape was under-reporting a user-visible number, not just a private one. And the Kotlin site atWearableExportImporter.kt:788is genuinelyefficiencyFromStages, the same computation as Swift'sefficiency(), so the parity claim in the table above is verified rather than assumed from the line numbers.