From f83b5b950b80f256e6a323a0a3b217d911ae9eda Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Mon, 31 Aug 2026 13:31:59 +1200 Subject: [PATCH 1/4] sleep: let the sparse bridge cross one short active run A brief wake ended the night. @justinjor-bit's report in #1657: up once for the bathroom, straight back to sleep, and a 6h40m night scored 150 minutes with eff=0.96 - a confident, clean, wrong answer that then fed the 21-day baseline every later night is judged against. Two mechanisms, both confirmed against the code and both fixed here. THE BRIDGE COULD NEVER REACH THE CASE IT WAS BUILT FOR. bridgeSparseSleep merged only runs already adjacent in its own output: an intervening active run was appended first, so the next pair was unreachable. A bathroom trip is definitionally an active run, so the rescue for fragmentation was structurally unavailable in the scenario that needs it most. The reporter's trace put a number on it - the bridge merged NOTHING on 14 of 14 sparse nights, runsAfter == runsBefore every time - and the pieces then died at the 60-minute session floor. One active run up to sparseBridgeActiveMaxMin is now absorbed, the whole span still subject to sparseBridgeGapMin and to the HR band. Two or more consecutive active runs are not: that is a night with structure in it, not one interruption. The bound is DEFINED as maxGapMin rather than given a number of its own. That is already this file's threshold for "a discontinuity this long is decisive", and reusing it keeps one such judgement in the detector instead of two that drift. It is also the cheap half of the guard: the real one is the HR band, which a genuine wake fails because it keeps HR elevated for its duration. mergeMin (15) already absorbs shorter active runs upstream in mergePeriods, so this covers the band between that and a wake long enough to be a real break. THE HR GATE USED THE WRONG STATISTIC. hrSleepBandAcross judged on the MEAN, which a single arousal spike drags out of band. confirmSleepWithHR, twelve lines below, already documents exactly this - "brief arousal / wake HR spikes (observed to ~190 bpm) ... pull the MEAN above baseline x mult and reject the run" - and uses the median for that reason. This gate asks the same question over a SHORTER window, where one spike dominates the mean even harder. Median now, which still rejects a SUSTAINED elevation just as firmly - that is the discrimination the gate exists to make, and without it removing the active-run blocker would not have helped the target case anyway. Also removes the shadow tracer. Swift kept a second copy of the bridge loop purely to emit trace lines, which had to be edited in step with the real one; the two are one pass now, so a trace cannot silently disagree with the behaviour it describes. Android had none of that machinery at all - the line that made this issue diagnosable was iOS-only, which is why the reporter being on iPhone is the reason we know any of this. Ported, and both platforms now report the intervening active run's length, which is the number the old trace could not supply. Two comments corrected rather than left. The bridge's own doc said "Active runs ... are left untouched; the span between two bridged sleep runs (an "active"/gap run, if present) is absorbed", which reads as though the case was handled - it was not, and that sentence is a large part of why this went unnoticed. And the empty-attempts trace line no longer claims "fragments separated by active runs", because that cause has just been removed; an empty list now means one sleep run, or fragments split by CONSECUTIVE active runs. One existing Swift test asserted "an intervening active run is never a considered pair". True when written, and it was the bug. Repinned to the same shape with the opposite expectation - the third test today that was correct and had quietly become a guard on the wrong behaviour. NOT changed: minSleepMin stays at 60. The reporter also shows 45-57 minute runs dropped by it, which is real, but it is a separate judgement about what counts as a session and it deserves its own evidence rather than riding along here. With the bridge working, fewer runs reach that floor fragmented in the first place. Android 4,935 tests and StrandAnalytics 1,669 (one skip), zero failures on both. Doc lint and the i18n gate clean. Not validated on a real interrupted night - the reporter has not caught one with the trace on yet, so the unit cases stand in for it. --- .../Sources/StrandAnalytics/SleepStager.swift | 162 ++++++++++++------ .../SleepStagerActiveBridgeTests.swift | 123 +++++++++++++ .../SparseBridgeAttemptTests.swift | 33 ++-- .../java/com/noop/analytics/SleepStager.kt | 160 +++++++++++++++-- .../analytics/SleepStagerActiveBridgeTest.kt | 141 +++++++++++++++ 5 files changed, 536 insertions(+), 83 deletions(-) create mode 100644 Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SleepStagerActiveBridgeTests.swift create mode 100644 android/app/src/test/java/com/noop/analytics/SleepStagerActiveBridgeTest.kt diff --git a/Packages/StrandAnalytics/Sources/StrandAnalytics/SleepStager.swift b/Packages/StrandAnalytics/Sources/StrandAnalytics/SleepStager.swift index 7be4760a44..a60a97f4fd 100644 --- a/Packages/StrandAnalytics/Sources/StrandAnalytics/SleepStager.swift +++ b/Packages/StrandAnalytics/Sources/StrandAnalytics/SleepStager.swift @@ -224,6 +224,17 @@ public enum SleepStager { /// floor (a real continuous night never has a true >90 min wake bridge mid-sleep). public static let sparseBridgeGapMin: Int = 90 + /// A single intervening ACTIVE run up to this long may be absorbed when bridging two sleep runs + /// (#1657). Deliberately DEFINED as `maxGapMin` rather than given a number of its own: that is + /// already this file's threshold for "a discontinuity this long is decisive", and reusing it means + /// there is one such judgement in the detector instead of two that can drift apart. + /// + /// The bound is the cheap half of the guard. The real one is the HR band across the whole span — a + /// genuine wake keeps HR elevated for its duration and fails it, while a brief stir does not. + /// `mergeMin` (15) already absorbs shorter active runs upstream in `mergePeriods`, so this covers the + /// band between that and a wake long enough to be a real break in the night. + public static let sparseBridgeActiveMaxMin: Int = maxGapMin + // MARK: - Stage 1–3 constants (sleep_features.py) public static let epochS: Double = 30.0 @@ -358,8 +369,15 @@ public enum SleepStager { guard let baseline = baseline else { return false } let seg = hr.filter { $0.ts > a && $0.ts <= b } if seg.isEmpty { return false } - let meanHR = Double(seg.reduce(0) { $0 + $1.bpm }) / Double(seg.count) - return meanHR <= baseline * hrSleepBandMult + // MEDIAN, not mean (#1657). `confirmSleepWithHR` below already documents why the mean is the + // wrong statistic here — "a real sleep night carries brief arousal / wake HR spikes (observed to + // ~190 bpm)" that drag it above the band — and uses the median for exactly that reason. This gate + // answers the same question over a SHORTER window, where a single spike dominates the mean even + // harder: a two-minute stir inside a fifteen-minute interval can put the mean out of band while + // the wearer was asleep for thirteen of those minutes. The median rejects a SUSTAINED elevation + // just as firmly, which is the discrimination this gate exists to make. + let medianHR = HRVAnalyzer.median(seg.map { Double($0.bpm) }) + return medianHR <= baseline * hrSleepBandMult } /// Per-record sleep flags from a rolling fraction of "still" samples. @@ -472,11 +490,13 @@ public enum SleepStager { struct SparseBridgeAttempt: Equatable, Sendable { /// Gap between the two runs, in whole minutes (negative when they overlap). let gapMin: Int + /// Minutes of intervening ACTIVE run, or 0 when the pair was only separated by a gap (#1657). + let activeMin: Int /// Whether the intervening HR stayed in the sleep band (the bridge's second condition). let hrInSleepBand: Bool /// Whether this pair was merged. let bridged: Bool - /// Stable token for the log: bridged / gapTooLong / hrOutOfBand / overlap. + /// Stable token for the log: bridged / gapTooLong / hrOutOfBand / overlap / activeTooLong. let reason: String } @@ -485,57 +505,90 @@ public enum SleepStager { /// actually happened rather than an approximation. Only pairs the bridge itself CONSIDERS (two /// adjacent sleep runs) produce an attempt; a pair separated by an active run is never considered, /// which is itself the answer when no attempts are reported. Pure — no I/O, no side effects. - static func sparseBridgeAttempts(_ periods: [Period], sparse: Bool, - hr: [HRSample], baseline: Double?) -> [SparseBridgeAttempt] { - guard sparse, !periods.isEmpty else { return [] } + /// The bridge, plus what it considered. See `bridgeSparseSleep` for the merge rule. + /// + /// #1657: an intervening ACTIVE run no longer blocks the merge permanently. The original loop could + /// only join runs already adjacent in its own output, so any active run between two sleep runs was + /// appended first and made the next pair unreachable — and a field trace found the bridge merging + /// NOTHING on 14 of 14 sparse nights for exactly that reason. Since a bathroom trip is definitionally + /// an active run, the rescue built for fragmentation was unavailable in the case that needs it most. + /// + /// A single active run up to `sparseBridgeActiveMaxMin` is now absorbed, with the whole span still + /// subject to `sparseBridgeGapMin` and to the HR band. Two or more consecutive active runs are not: + /// that is a night with real structure in it, not one interruption. + /// + /// This USED to be a shadow copy of the loop kept only for tracing, which had to be edited in step + /// with the real one — a trace that quietly disagrees with the behaviour it describes is worse than + /// no trace. Merge and trace are one pass now. Kotlin twin: `bridgeSparseSleepTraced`. + static func bridgeSparseSleepTraced(_ periods: [Period], sparse: Bool, hr: [HRSample], + baseline: Double?) -> ([Period], [SparseBridgeAttempt]) { + if !sparse || periods.isEmpty { return (periods, []) } let bridgeGapS = sparseBridgeGapMin * 60 - var attempts: [SparseBridgeAttempt] = [] + let activeMaxS = sparseBridgeActiveMaxMin * 60 var out: [Period] = [] + var attempts: [SparseBridgeAttempt] = [] + + /// Judge one candidate pair, record it, and merge when it passes. + /// + /// The order of the checks fixes which reason a failing pair reports, and it is deliberate: + /// overlap and gap are properties of the pair, activeTooLong is a property of what sits between + /// them, and hrOutOfBand is last because it is the only one that needed the HR series to decide. + func consider(_ left: Period, _ right: Period, activeS: Int, + dropTrailing: Bool, activeMaxS: Int) -> Bool { + let gap = right.start - left.end + let inBand = hrSleepBandAcross(left.end, right.start, hr: hr, baseline: baseline) + let reason: String + if gap < 0 { reason = "overlap" } + else if gap > bridgeGapS { reason = "gapTooLong" } + else if activeS > activeMaxS { reason = "activeTooLong" } + else if !inBand { reason = "hrOutOfBand" } + else { reason = "bridged" } + let bridged = reason == "bridged" + attempts.append(SparseBridgeAttempt(gapMin: gap / 60, activeMin: activeS / 60, + hrInSleepBand: inBand, bridged: bridged, reason: reason)) + guard bridged else { return false } + if dropTrailing { out.removeLast() } + out[out.count - 1] = Period(stage: "sleep", start: left.start, end: right.end) + return true + } + for p in periods { - if let last = out.last, last.stage == "sleep", p.stage == "sleep" { - let gap = p.start - last.end - let inBand = hrSleepBandAcross(last.end, p.start, hr: hr, baseline: baseline) - let bridged = gap >= 0 && gap <= bridgeGapS && inBand - let reason: String - if bridged { reason = "bridged" } - else if gap < 0 { reason = "overlap" } - else if gap > bridgeGapS { reason = "gapTooLong" } - else { reason = "hrOutOfBand" } - attempts.append(SparseBridgeAttempt(gapMin: gap / 60, hrInSleepBand: inBand, - bridged: bridged, reason: reason)) - if bridged { - out[out.count - 1] = Period(stage: "sleep", start: last.start, end: p.end) - continue + if p.stage == "sleep" { + // Case 1: the previous run is sleep — the original adjacent-pair merge. + if let last = out.last, last.stage == "sleep" { + if consider(last, p, activeS: 0, dropTrailing: false, activeMaxS: 0) { continue } + } + // Case 2: exactly one active run sits between two sleep runs. Absorbed when short + // enough, which is the #1657 case the original loop could never reach. + if out.count >= 2, let last = out.last, last.stage == "active", + out[out.count - 2].stage == "sleep" { + let prev = out[out.count - 2] + if consider(prev, p, activeS: last.end - last.start, + dropTrailing: true, activeMaxS: activeMaxS) { continue } } } out.append(p) } - return attempts + return (out, attempts) } - /// Sparse-gravity bridge (#308): merge two adjacent SLEEP runs separated ONLY by a gap up to - /// sparseBridgeGapMin minutes when the intervening HR stays in the sleep band — so a real night - /// fragmented by gravity dropouts is re-stitched into one continuous in-bed span BEFORE the - /// minSleepMin gate drops the pieces. Active runs and over-threshold gaps are left untouched; - /// the span between two bridged sleep runs (an "active"/gap run, if present) is absorbed. - /// A no-op when `sparse == false`, so the dense 4.0 path is unchanged. + /// Sparse-gravity bridge (#308): merge two SLEEP runs when the intervening HR stays in the sleep + /// band — so a real night fragmented by gravity dropouts is re-stitched into one continuous in-bed + /// span BEFORE the minSleepMin gate drops the pieces. A no-op when `sparse == false`, so the dense + /// 4.0 path is unchanged. + /// + /// What sits between the two runs may be a bare gap (up to `sparseBridgeGapMin`) or ONE active run + /// (additionally up to `sparseBridgeActiveMaxMin`). Over-threshold gaps, longer active runs and two + /// consecutive active runs are all left untouched. + /// + /// The previous wording here — "Active runs … are left untouched; the span between two bridged sleep + /// runs (an "active"/gap run, if present) is absorbed" — read as though an intervening active run was + /// already handled. It was not: only a bare gap was, and that sentence is a large part of why #1657 + /// went unnoticed. Kept in the history rather than quietly deleted, because the next person to widen + /// this function will read this comment first. static func bridgeSparseSleep(_ periods: [Period], sparse: Bool, hr: [HRSample], baseline: Double?) -> [Period] { - if !sparse || periods.isEmpty { return periods } - let bridgeGapS = sparseBridgeGapMin * 60 - var out: [Period] = [] - for p in periods { - if let last = out.last, last.stage == "sleep", p.stage == "sleep" { - let gap = p.start - last.end - if gap >= 0 && gap <= bridgeGapS - && hrSleepBandAcross(last.end, p.start, hr: hr, baseline: baseline) { - out[out.count - 1] = Period(stage: "sleep", start: last.start, end: p.end) - continue - } - } - out.append(p) - } - return out + bridgeSparseSleepTraced(periods, sparse: sparse, hr: hr, baseline: baseline).0 } // MARK: - HR refinement @@ -1048,9 +1101,9 @@ public enum SleepStager { let runsBeforeBridge = traceSink == nil ? 0 : runs.filter { $0.stage == "sleep" }.count // #737: capture the per-pair reasons BEFORE the merge mutates `runs`, so a bridge that changed // nothing still says why (gapTooLong / hrOutOfBand / overlap) instead of only before==after. - let bridgeAttempts = traceSink == nil ? [] : sparseBridgeAttempts(runs, sparse: sparse, hr: hrS, - baseline: baseline) - runs = bridgeSparseSleep(runs, sparse: sparse, hr: hrS, baseline: baseline) + let bridgeResult = bridgeSparseSleepTraced(runs, sparse: sparse, hr: hrS, baseline: baseline) + let bridgeAttempts = bridgeResult.1 + runs = bridgeResult.0 // Sleep & Rest test mode (E3): record the sparse-gravity bridge result, so a sparse 5.0 night // rescued from fragmentation is visible. Only emitted when gravity is sparse (the only case the // bridge can act) and only when tracing. Side-effect-only. @@ -1059,19 +1112,24 @@ public enum SleepStager { traceSink(GateTrace.runLine(index: -1, startTs: 0, endTs: 0, verdict: runsAfterBridge < runsBeforeBridge ? .kept : .dropped, gate: "sparseBridge", detail: "sparse=true gapMin=\(sparseBridgeGapMin) runsBefore=\(runsBeforeBridge) runsAfter=\(runsAfterBridge)")) - // #737: one line per pair the bridge CONSIDERED. No lines at all means no two adjacent sleep - // runs were ever seen — i.e. the fragments are separated by active runs, which the bridge - // deliberately never crosses. That absence is itself the diagnosis. + // #737: one line per pair the bridge CONSIDERED, each naming what it decided. + // + // #1657 changed what an empty list MEANS, so the wording changed with it. It used to mean + // "the fragments are separated by active runs", because such a pair could never be reached — + // that absence was the diagnosis. A single short active run is now a considered pair, so an + // empty list can only mean there was no candidate at all: one sleep run, or fragments split + // by two or more consecutive active runs. Leaving the old text would have pointed the next + // reader at a cause that had just been removed. if bridgeAttempts.isEmpty { traceSink(GateTrace.runLine(index: -1, startTs: 0, endTs: 0, verdict: .dropped, gate: "sparseBridge", - detail: "no adjacent sleep pairs considered (fragments separated by active runs)")) + detail: "no candidate pairs (one sleep run, or fragments split by consecutive active runs)")) } for (i, a) in bridgeAttempts.enumerated() { traceSink(GateTrace.runLine(index: -1, startTs: 0, endTs: 0, verdict: a.bridged ? .kept : .dropped, gate: "sparseBridgePair", - detail: "pair=\(i) gapMin=\(a.gapMin) hrInSleepBand=\(a.hrInSleepBand) " - + "reason=\(a.reason)")) + detail: "pair=\(i) gapMin=\(a.gapMin) activeMin=\(a.activeMin) " + + "hrInSleepBand=\(a.hrInSleepBand) reason=\(a.reason)")) } } diff --git a/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SleepStagerActiveBridgeTests.swift b/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SleepStagerActiveBridgeTests.swift new file mode 100644 index 0000000000..4649693669 --- /dev/null +++ b/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SleepStagerActiveBridgeTests.swift @@ -0,0 +1,123 @@ +import XCTest +@testable import StrandAnalytics +import WhoopProtocol + +/// #1657 twin of `SleepStagerActiveBridgeTest`. The sparse-gravity bridge could only ever join sleep +/// runs already adjacent in its own output, so ANY active run between two sleep runs blocked the merge +/// permanently. A field trace found it merging nothing on 14 of 14 sparse nights for exactly that reason +/// — and since a bathroom trip is definitionally an active run, the rescue built for fragmentation was +/// unavailable in the case that needs it most. The pieces then died at the 60-minute session floor, +/// which is how a 6h40m night scored 150 minutes. +final class SleepStagerActiveBridgeTests: XCTestCase { + + private func sleep(_ start: Int, _ end: Int) -> SleepStager.Period { + SleepStager.Period(stage: "sleep", start: start, end: end) + } + private func active(_ start: Int, _ end: Int) -> SleepStager.Period { + SleepStager.Period(stage: "active", start: start, end: end) + } + /// Flat HR well under the band, so the HR gate is never the thing under test. + private func calmHr(_ from: Int, _ to: Int, bpm: Int = 50) -> [HRSample] { + stride(from: from, through: to, by: 60).map { HRSample(ts: $0, bpm: bpm) } + } + private let baseline = 60.0 + + /// THE reported shape: asleep, a short trip, asleep again. Each piece is under the 60-minute session + /// floor on its own; together they are a night. + func testAShortActiveInterruptionBetweenTwoSleepRunsIsAbsorbed() { + let out = SleepStager.bridgeSparseSleep( + [sleep(0, 3000), active(3000, 3900), sleep(3900, 9000)], + sparse: true, hr: calmHr(0, 9000), baseline: baseline) + XCTAssertEqual(out.count, 1) + XCTAssertEqual(out.first?.stage, "sleep") + XCTAssertEqual(out.first?.start, 0) + XCTAssertEqual(out.first?.end, 9000) + } + + /// The guard that keeps this honest. A long active run is a real break in the night, not a stir, and + /// absorbing it would score wakefulness as sleep — wrong in a new direction and harder to notice than + /// the truncation being fixed. + func testAnActiveRunLongerThanTheBoundIsLeftAlone() { + let tooLong = SleepStager.sparseBridgeActiveMaxMin * 60 + 60 + let out = SleepStager.bridgeSparseSleep( + [sleep(0, 3000), active(3000, 3000 + tooLong), sleep(3000 + tooLong, 9000)], + sparse: true, hr: calmHr(0, 9000), baseline: baseline) + XCTAssertEqual(out.count, 3) + } + + /// HR is the real gate, not the duration bound. A wearer who is genuinely up keeps HR elevated for + /// the whole interruption, and that must still block the merge even when it is short. + func testAShortInterruptionWithElevatedHRIsNotAbsorbed() { + let hot = calmHr(0, 3000) + calmHr(3001, 3900, bpm: 110) + calmHr(3901, 9000) + let out = SleepStager.bridgeSparseSleep( + [sleep(0, 3000), active(3000, 3900), sleep(3900, 9000)], + sparse: true, hr: hot, baseline: baseline) + XCTAssertEqual(out.count, 3) + } + + /// Two consecutive active runs are a night with structure in it, not one interruption. + func testTwoConsecutiveActiveRunsAreNotAbsorbed() { + let out = SleepStager.bridgeSparseSleep( + [sleep(0, 3000), active(3000, 3300), active(3300, 3900), sleep(3900, 9000)], + sparse: true, hr: calmHr(0, 9000), baseline: baseline) + XCTAssertEqual(out.count, 4) + } + + /// A dense 4.0 night must be byte-identical: the bridge is sparse-only and always has been. + /// `Period` is not Equatable in production, so the fields are compared rather than widening a + /// production type for a test's convenience. + func testADenseNightIsUntouched() { + let periods = [sleep(0, 3000), active(3000, 3900), sleep(3900, 9000)] + let out = SleepStager.bridgeSparseSleep(periods, sparse: false, hr: calmHr(0, 9000), + baseline: baseline) + XCTAssertEqual(out.map(\.stage), periods.map(\.stage)) + XCTAssertEqual(out.map(\.start), periods.map(\.start)) + XCTAssertEqual(out.map(\.end), periods.map(\.end)) + } + + /// The pre-existing behaviour — a bare gap between two sleep runs — still merges. + func testTheOriginalAdjacentPairMergeStillWorks() { + let out = SleepStager.bridgeSparseSleep( + [sleep(0, 3000), sleep(3600, 9000)], + sparse: true, hr: calmHr(0, 9000), baseline: baseline) + XCTAssertEqual(out.count, 1) + XCTAssertEqual(out.first?.end, 9000) + } + + /// The trace has to say WHY, and the blocking length is the number a reader needs. The old trace could + /// only report runsBefore == runsAfter, which says the bridge did nothing and not what stopped it. + func testABlockedPairReportsTheBoundThatBlockedItWithTheActiveLength() { + let tooLong = SleepStager.sparseBridgeActiveMaxMin * 60 + 60 + let (_, attempts) = SleepStager.bridgeSparseSleepTraced( + [sleep(0, 3000), active(3000, 3000 + tooLong), sleep(3000 + tooLong, 9000)], + sparse: true, hr: calmHr(0, 9000), baseline: baseline) + XCTAssertEqual(attempts.count, 1) + XCTAssertEqual(attempts.first?.reason, "activeTooLong") + XCTAssertEqual(attempts.first?.bridged, false) + XCTAssertEqual(attempts.first?.activeMin, SleepStager.sparseBridgeActiveMaxMin + 1) + } + + /// The tracer and the merge are ONE pass now. This file used to keep a shadow copy of the loop purely + /// to trace it, which had to be edited in step with the real one — a trace that quietly disagrees + /// with the behaviour it describes is worse than no trace at all. + func testTheTracedPassReturnsExactlyWhatThePlainOneDoes() { + let periods = [sleep(0, 3000), active(3000, 3900), sleep(3900, 9000)] + let hr = calmHr(0, 9000) + let plain = SleepStager.bridgeSparseSleep(periods, sparse: true, hr: hr, baseline: baseline) + let traced = SleepStager.bridgeSparseSleepTraced(periods, sparse: true, hr: hr, + baseline: baseline).0 + XCTAssertEqual(plain.map(\.stage), traced.map(\.stage)) + XCTAssertEqual(plain.map(\.start), traced.map(\.start)) + XCTAssertEqual(plain.map(\.end), traced.map(\.end)) + } + + /// #1657, the other half: `hrSleepBandAcross` judged on the MEAN, which a single arousal spike drags + /// out of band — the exact statistic `confirmSleepWithHR` documents as wrong for this, and uses the + /// median for instead. A sustained elevation must still be rejected. + func testABriefSpikeNoLongerPutsTheWholeIntervalOutOfBandButASustainedOneDoes() { + let spiky = calmHr(0, 3540) + calmHr(3541, 3660, bpm: 190) + XCTAssertTrue(SleepStager.hrSleepBandAcross(0, 3660, hr: spiky, baseline: baseline)) + XCTAssertFalse(SleepStager.hrSleepBandAcross(0, 3660, hr: calmHr(0, 3660, bpm: 110), + baseline: baseline)) + } +} diff --git a/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SparseBridgeAttemptTests.swift b/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SparseBridgeAttemptTests.swift index 2a5f5f73ae..b42ddd4edc 100644 --- a/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SparseBridgeAttemptTests.swift +++ b/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SparseBridgeAttemptTests.swift @@ -23,7 +23,7 @@ final class SparseBridgeAttemptTests: XCTestCase { /// must equal the reduction in sleep-run count the REAL bridge produces. private func assertAgreesWithBridge(_ periods: [SleepStager.Period], hr: [HRSample], baseline: Double?, file: StaticString = #filePath, line: UInt = #line) { - let attempts = SleepStager.sparseBridgeAttempts(periods, sparse: true, hr: hr, baseline: baseline) + let attempts = SleepStager.bridgeSparseSleepTraced(periods, sparse: true, hr: hr, baseline: baseline).1 let before = periods.filter { $0.stage == "sleep" }.count let after = SleepStager.bridgeSparseSleep(periods, sparse: true, hr: hr, baseline: baseline) .filter { $0.stage == "sleep" }.count @@ -35,7 +35,7 @@ final class SparseBridgeAttemptTests: XCTestCase { // Two sleep runs 10 min apart, HR asleep across the gap → bridged. let periods = [sleep(0, 3_600), sleep(4_200, 8_000)] let hr = sleepHR(from: 0, to: 8_000) - let a = SleepStager.sparseBridgeAttempts(periods, sparse: true, hr: hr, baseline: 70) + let a = SleepStager.bridgeSparseSleepTraced(periods, sparse: true, hr: hr, baseline: 70).1 XCTAssertEqual(a.count, 1) XCTAssertEqual(a[0].gapMin, 10) XCTAssertTrue(a[0].bridged) @@ -48,7 +48,7 @@ final class SparseBridgeAttemptTests: XCTestCase { let gap = (SleepStager.sparseBridgeGapMin + 30) * 60 let periods = [sleep(0, 3_600), sleep(3_600 + gap, 3_600 + gap + 3_600)] let hr = sleepHR(from: 0, to: 3_600 + gap + 3_600) - let a = SleepStager.sparseBridgeAttempts(periods, sparse: true, hr: hr, baseline: 70) + let a = SleepStager.bridgeSparseSleepTraced(periods, sparse: true, hr: hr, baseline: 70).1 XCTAssertEqual(a.count, 1) XCTAssertFalse(a[0].bridged) XCTAssertEqual(a[0].reason, "gapTooLong") @@ -60,7 +60,7 @@ final class SparseBridgeAttemptTests: XCTestCase { // Gap is short enough, but HR across it is clearly awake → the HR condition is what refused. let periods = [sleep(0, 3_600), sleep(4_200, 8_000)] let hr = sleepHR(from: 0, to: 8_000, bpm: 110) - let a = SleepStager.sparseBridgeAttempts(periods, sparse: true, hr: hr, baseline: 60) + let a = SleepStager.bridgeSparseSleepTraced(periods, sparse: true, hr: hr, baseline: 60).1 XCTAssertEqual(a.count, 1) XCTAssertFalse(a[0].bridged) XCTAssertFalse(a[0].hrInSleepBand) @@ -68,21 +68,28 @@ final class SparseBridgeAttemptTests: XCTestCase { assertAgreesWithBridge(periods, hr: hr, baseline: 60) } - /// The reporter's shape (#737): fragments separated by ACTIVE runs are never adjacent sleep pairs, so - /// the bridge considers nothing — which is exactly why their log showed runsBefore == runsAfter with - /// no explanation. An EMPTY attempt list is the diagnosis, and the emitter says so in words. - func testFragmentsSeparatedByActiveRunsProduceNoAttempts() { + /// REPINNED for #1657. This used to assert the opposite — "an intervening active run is never a + /// considered pair" — which was true, and was the bug: the shape it describes is the reporter's + /// bathroom break, and the bridge built to rescue fragmentation was structurally unable to reach it. + /// A field trace then found the bridge merging nothing on 14 of 14 sparse nights for this reason. + /// + /// The test was correct when written and quietly became a guard on the wrong behaviour. Kept pointing + /// at the same shape so the history reads straight. + func testAShortActiveRunBetweenFragmentsIsNowConsideredAndBridged() { let periods = [sleep(0, 3_000), active(3_000, 3_300), sleep(3_300, 6_000)] let hr = sleepHR(from: 0, to: 6_000) - let a = SleepStager.sparseBridgeAttempts(periods, sparse: true, hr: hr, baseline: 70) - XCTAssertTrue(a.isEmpty, "an intervening active run is never a considered pair") - assertAgreesWithBridge(periods, hr: hr, baseline: 70) // and the real bridge merges nothing + let (out, a) = SleepStager.bridgeSparseSleepTraced(periods, sparse: true, hr: hr, baseline: 70) + XCTAssertEqual(a.count, 1, "the pair is now considered") + XCTAssertEqual(a.first?.reason, "bridged") + XCTAssertEqual(a.first?.activeMin, 5) + XCTAssertEqual(out.count, 1, "and the night is one run again") + XCTAssertEqual(out.first?.end, 6_000) } func testNoOpWhenNotSparse() { let periods = [sleep(0, 3_600), sleep(4_200, 8_000)] let hr = sleepHR(from: 0, to: 8_000) - XCTAssertTrue(SleepStager.sparseBridgeAttempts(periods, sparse: false, hr: hr, baseline: 70).isEmpty, + XCTAssertTrue(SleepStager.bridgeSparseSleepTraced(periods, sparse: false, hr: hr, baseline: 70).1.isEmpty, "the dense 4.0 path is untouched, so there is nothing to explain") } @@ -91,7 +98,7 @@ final class SparseBridgeAttemptTests: XCTestCase { func testChainOfThreeReportsTwoPairsAndAgrees() { let periods = [sleep(0, 3_600), sleep(4_200, 8_000), sleep(8_600, 12_000)] let hr = sleepHR(from: 0, to: 12_000) - let a = SleepStager.sparseBridgeAttempts(periods, sparse: true, hr: hr, baseline: 70) + let a = SleepStager.bridgeSparseSleepTraced(periods, sparse: true, hr: hr, baseline: 70).1 XCTAssertEqual(a.count, 2) XCTAssertTrue(a.allSatisfy { $0.bridged }) assertAgreesWithBridge(periods, hr: hr, baseline: 70) diff --git a/android/app/src/main/java/com/noop/analytics/SleepStager.kt b/android/app/src/main/java/com/noop/analytics/SleepStager.kt index 5a2067fbe6..9698ca03d2 100644 --- a/android/app/src/main/java/com/noop/analytics/SleepStager.kt +++ b/android/app/src/main/java/com/noop/analytics/SleepStager.kt @@ -320,6 +320,19 @@ object SleepStager { */ const val sparseBridgeGapMin: Int = 90 + /** + * A single intervening ACTIVE run up to this long may be absorbed when bridging two sleep runs + * (#1657). Deliberately DEFINED as [maxGapMin] rather than given a number of its own: that is + * already this file's threshold for "a discontinuity this long is decisive", and reusing it means + * there is one such judgement in the detector instead of two that can drift apart. + * + * The bound is the cheap half of the guard. The real one is the HR band across the whole span — + * a genuine wake keeps HR elevated for its duration and fails it, while a brief stir does not. + * [mergeMin] (15) already absorbs shorter active runs upstream in [mergePeriods], so this covers + * the band between that and a wake long enough to be a real break in the night. + */ + const val sparseBridgeActiveMaxMin: Int = maxGapMin + // ── Stage 1–3 constants (sleep_features.py) ────────────────────────────── const val epochS: Double = 30.0 @@ -471,8 +484,15 @@ object SleepStager { if (baseline == null) return false val seg = hr.filter { it.ts > a && it.ts <= b } if (seg.isEmpty()) return false - val meanHR = seg.sumOf { it.bpm }.toDouble() / seg.size.toDouble() - return meanHR <= baseline * hrSleepBandMult + // MEDIAN, not mean (#1657). [confirmSleepWithHR] twelve lines below already documents why the + // mean is the wrong statistic here — "a real sleep night carries brief arousal / wake HR spikes + // (observed to ~190 bpm)" that drag it above the band — and uses the median for exactly that + // reason. This gate answers the same question over a SHORTER window, where a single spike + // dominates the mean even harder: a two-minute stir inside a fifteen-minute interval can put the + // mean out of band while the wearer was asleep for thirteen of those minutes. The median rejects + // a SUSTAINED elevation just as firmly, which is the discrimination this gate exists to make. + val medianHR = HrvAnalyzer.median(seg.map { it.bpm.toDouble() }) + return medianHR <= baseline * hrSleepBandMult } /** Per-record sleep flags from a rolling fraction of "still" samples. */ @@ -594,31 +614,116 @@ object SleepStager { } /** - * Sparse-gravity bridge (#308): merge two adjacent SLEEP runs separated ONLY by a gap up to - * sparseBridgeGapMin minutes when the intervening HR stays in the sleep band — so a real night - * fragmented by gravity dropouts is re-stitched into one continuous in-bed span BEFORE the - * minSleepMin gate drops the pieces. Active runs and over-threshold gaps are left untouched; the - * span between two bridged sleep runs (an "active"/gap run, if present) is absorbed. A no-op when - * [sparse] == false, so the dense 4.0 path is unchanged. Mirrors Swift. + * Sparse-gravity bridge (#308): merge two SLEEP runs when the intervening HR stays in the sleep + * band — so a real night fragmented by gravity dropouts is re-stitched into one continuous in-bed + * span BEFORE the minSleepMin gate drops the pieces. A no-op when [sparse] == false, so the dense + * 4.0 path is unchanged. Mirrors Swift. + * + * What sits between the two runs may be a bare gap (up to [sparseBridgeGapMin]) or ONE active run + * (additionally up to [sparseBridgeActiveMaxMin]). Over-threshold gaps, longer active runs and two + * consecutive active runs are all left untouched. + * + * The previous wording here — "Active runs … are left untouched; the span between two bridged sleep + * runs (an "active"/gap run, if present) is absorbed" — read as though an intervening active run was + * already handled. It was not: only a bare gap was, and that sentence is a large part of why #1657 + * went unnoticed. Kept in the history rather than quietly deleted, because the next person to widen + * this function will read this comment first. */ internal fun bridgeSparseSleep( periods: List, sparse: Boolean, hr: List, baseline: Double?, - ): List { - if (!sparse || periods.isEmpty()) return periods + ): List = bridgeSparseSleepTraced(periods, sparse, hr, baseline).first + + /** + * One pair the bridge looked at, for the Sleep & Rest trace. Kept beside the merge itself rather + * than recomputed by a shadow pass: a second implementation of this loop would have to be edited in + * step with the first, and a trace that silently disagrees with the behaviour it describes is worse + * than no trace. Mirrors Swift `SparseBridgeAttempt`. + */ + internal data class SparseBridgeAttempt( + /** Gap between the two sleep runs, whole minutes (negative when they overlap). */ + val gapMin: Long, + /** Minutes of intervening ACTIVE run, or 0 when the pair was only separated by a gap. */ + val activeMin: Long, + /** Whether the intervening HR stayed in the sleep band. */ + val hrInSleepBand: Boolean, + /** Whether this pair was merged. */ + val bridged: Boolean, + /** Stable token: bridged / gapTooLong / hrOutOfBand / overlap / activeTooLong. */ + val reason: String, + ) + + /** + * The bridge, plus what it considered. See [bridgeSparseSleep] for the merge rule. + * + * #1657: an intervening ACTIVE run no longer blocks the merge permanently. The original loop could + * only join runs already adjacent in its own output, so any active run between two sleep runs was + * appended first and made the next pair unreachable — and a field trace found the bridge merging + * NOTHING on 14 of 14 sparse nights for exactly that reason. Since a bathroom trip is definitionally + * an active run, the rescue built for fragmentation was unavailable in the case that needs it most. + * + * A single active run up to [sparseBridgeActiveMaxMin] is now absorbed, with the whole span still + * subject to [sparseBridgeGapMin] and to the HR band. Two or more consecutive active runs are not: + * that is a night with real structure in it, not one interruption. + */ + internal fun bridgeSparseSleepTraced( + periods: List, sparse: Boolean, hr: List, baseline: Double?, + ): Pair, List> { + if (!sparse || periods.isEmpty()) return periods to emptyList() val bridgeGapS = (sparseBridgeGapMin * 60).toLong() + val activeMaxS = (sparseBridgeActiveMaxMin * 60).toLong() val out = ArrayList() + val attempts = ArrayList() for (p in periods) { - val last = out.lastOrNull() - if (last != null && last.stage == "sleep" && p.stage == "sleep") { - val gap = p.start - last.end - if (gap in 0..bridgeGapS && hrSleepBandAcross(last.end, p.start, hr, baseline)) { - out[out.size - 1] = Period(stage = "sleep", start = last.start, end = p.end) - continue + if (p.stage == "sleep") { + val last = out.lastOrNull() + // Case 1: the previous run is sleep — the original adjacent-pair merge. + if (last != null && last.stage == "sleep") { + if (considerBridge(out, attempts, last, p, activeS = 0L, bridgeGapS, hr, baseline)) continue + } + // Case 2: exactly one active run sits between two sleep runs. Absorbed when short + // enough, which is the #1657 case the original loop could never reach. + val prev = if (out.size >= 2) out[out.size - 2] else null + if (last != null && last.stage == "active" && prev != null && prev.stage == "sleep") { + val activeS = last.end - last.start + if (considerBridge(out, attempts, prev, p, activeS, bridgeGapS, hr, baseline, + dropTrailing = true, activeMaxS = activeMaxS)) continue } } out.add(p) } - return out + return out to attempts + } + + /** + * Judge one candidate pair, record it, and merge when it passes. Returns true when [p] was absorbed. + * + * [dropTrailing] removes the intervening active run from [out] before merging — the whole point of + * case 2. The order of the checks fixes which reason a failing pair reports, and it is deliberate: + * overlap and gap are properties of the pair, activeTooLong is a property of what sits between them, + * and hrOutOfBand is last because it is the only one that needed the HR series to decide. + */ + private fun considerBridge( + out: ArrayList, attempts: ArrayList, + left: Period, right: Period, activeS: Long, bridgeGapS: Long, + hr: List, baseline: Double?, + dropTrailing: Boolean = false, activeMaxS: Long = 0L, + ): Boolean { + val gap = right.start - left.end + val inBand = hrSleepBandAcross(left.end, right.start, hr, baseline) + val reason = when { + gap < 0 -> "overlap" + gap > bridgeGapS -> "gapTooLong" + activeS > activeMaxS -> "activeTooLong" + !inBand -> "hrOutOfBand" + else -> "bridged" + } + val bridged = reason == "bridged" + attempts.add(SparseBridgeAttempt(gapMin = gap / 60, activeMin = activeS / 60, + hrInSleepBand = inBand, bridged = bridged, reason = reason)) + if (!bridged) return false + if (dropTrailing) out.removeAt(out.size - 1) + out[out.size - 1] = Period(stage = "sleep", start = left.start, end = right.end) + return true } // ── HR refinement ──────────────────────────────────────────────────────── @@ -1148,7 +1253,8 @@ object SleepStager { runs = mergePeriods(runs) // Re-stitch sleep runs fragmented by pure gravity dropouts (sparse only) before minSleepMin. val runsBeforeBridge = if (traceSink == null) 0 else runs.count { it.stage == "sleep" } - runs = bridgeSparseSleep(runs, sparse = sparse, hr = hrS, baseline = baseline) + val bridged = bridgeSparseSleepTraced(runs, sparse = sparse, hr = hrS, baseline = baseline) + runs = bridged.first // Sleep & Rest test mode (E10): record the sparse-gravity bridge result, so a sparse 5.0 night // rescued from fragmentation is visible. Only when gravity is sparse and only when tracing. if (traceSink != null && sparse) { @@ -1156,6 +1262,24 @@ object SleepStager { traceSink(SleepStagerTrace.runLine(-1, 0, 0, if (runsAfterBridge < runsBeforeBridge) SleepStagerTrace.Verdict.KEPT else SleepStagerTrace.Verdict.DROPPED, "sparseBridge", "sparse=true gapMin=$sparseBridgeGapMin runsBefore=$runsBeforeBridge runsAfter=$runsAfterBridge")) + // #1657: one line per pair the bridge CONSIDERED, ported from Swift and extended with the + // intervening active run's length. No lines at all still means no candidate pair existed; + // an activeTooLong line names the bound that blocked one, which is the number a future + // reader needs and the old trace could not supply. Android had NONE of this before — the + // line that made #1657 diagnosable was iOS-only, so an Android reporter saw runsBefore == + // runsAfter and no reason at all. + if (bridged.second.isEmpty()) { + traceSink(SleepStagerTrace.runLine(-1, 0, 0, SleepStagerTrace.Verdict.DROPPED, + "sparseBridge", + "no candidate pairs (one sleep run, or fragments split by consecutive active runs)")) + } + for ((i, a) in bridged.second.withIndex()) { + traceSink(SleepStagerTrace.runLine(-1, 0, 0, + if (a.bridged) SleepStagerTrace.Verdict.KEPT else SleepStagerTrace.Verdict.DROPPED, + "sparseBridgePair", + "pair=$i gapMin=${a.gapMin} activeMin=${a.activeMin} " + + "hrInSleepBand=${a.hrInSleepBand} reason=${a.reason}")) + } } val minSleepS = (minSleepMin * 60).toLong() diff --git a/android/app/src/test/java/com/noop/analytics/SleepStagerActiveBridgeTest.kt b/android/app/src/test/java/com/noop/analytics/SleepStagerActiveBridgeTest.kt new file mode 100644 index 0000000000..63a86106b8 --- /dev/null +++ b/android/app/src/test/java/com/noop/analytics/SleepStagerActiveBridgeTest.kt @@ -0,0 +1,141 @@ +package com.noop.analytics + +import com.noop.data.HrSample +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * #1657: the sparse-gravity bridge could only ever join sleep runs already adjacent in its own output, + * so ANY active run between two sleep runs blocked the merge permanently. A field trace found it merging + * nothing on 14 of 14 sparse nights for exactly that reason — and since a bathroom trip is definitionally + * an active run, the rescue built for fragmentation was unavailable in the case that needs it most. + * + * The pieces then died at the 60-minute session floor, which is how a 6h40m night scored 150 minutes. + */ +class SleepStagerActiveBridgeTest { + + private fun sleep(start: Long, end: Long) = SleepStager.Period("sleep", start, end) + private fun active(start: Long, end: Long) = SleepStager.Period("active", start, end) + + /** Flat HR well under the band, so the HR gate is never the thing under test. */ + private fun calmHr(from: Long, to: Long, bpm: Int = 50): List = + (from..to step 60).map { HrSample(deviceId = "dev", ts = it, bpm = bpm) } + + private val baseline = 60.0 + + /** + * THE reported shape: asleep, a short trip, asleep again. Each piece is under the 60-minute session + * floor on its own; together they are a night. + */ + @Test + fun `a short active interruption between two sleep runs is absorbed`() { + val periods = listOf(sleep(0, 3000), active(3000, 3900), sleep(3900, 9000)) + val out = SleepStager.bridgeSparseSleep(periods, sparse = true, hr = calmHr(0, 9000), baseline = baseline) + assertEquals(1, out.size) + assertEquals("sleep", out[0].stage) + assertEquals(0L, out[0].start) + assertEquals(9000L, out[0].end) + } + + /** + * The guard that keeps this honest. A long active run is a real break in the night, not a stir, and + * absorbing it would score wakefulness as sleep — wrong in a new direction and harder to notice than + * the truncation being fixed. + */ + @Test + fun `an active run longer than the bound is left alone`() { + val tooLong = (SleepStager.sparseBridgeActiveMaxMin * 60L) + 60L + val periods = listOf(sleep(0, 3000), active(3000, 3000 + tooLong), sleep(3000 + tooLong, 9000)) + val out = SleepStager.bridgeSparseSleep(periods, sparse = true, hr = calmHr(0, 9000), baseline = baseline) + assertEquals(3, out.size) + } + + /** + * HR is the real gate, not the duration bound. A wearer who is genuinely up keeps HR elevated for the + * whole interruption, and that must still block the merge even when it is short. + */ + @Test + fun `a short interruption with elevated HR is not absorbed`() { + val periods = listOf(sleep(0, 3000), active(3000, 3900), sleep(3900, 9000)) + val hot = calmHr(0, 3000) + calmHr(3001, 3900, bpm = 110) + calmHr(3901, 9000) + val out = SleepStager.bridgeSparseSleep(periods, sparse = true, hr = hot, baseline = baseline) + assertEquals(3, out.size) + } + + /** + * Two consecutive active runs are a night with structure in it, not one interruption. Only a single + * intervening run is absorbed, or the bridge would walk across an arbitrarily fragmented evening. + */ + @Test + fun `two consecutive active runs are not absorbed`() { + val periods = listOf( + sleep(0, 3000), active(3000, 3300), active(3300, 3900), sleep(3900, 9000), + ) + val out = SleepStager.bridgeSparseSleep(periods, sparse = true, hr = calmHr(0, 9000), baseline = baseline) + assertEquals(4, out.size) + } + + /** A dense 4.0 night must be byte-identical: the bridge is sparse-only and always has been. */ + @Test + fun `a dense night is untouched`() { + val periods = listOf(sleep(0, 3000), active(3000, 3900), sleep(3900, 9000)) + val out = SleepStager.bridgeSparseSleep(periods, sparse = false, hr = calmHr(0, 9000), baseline = baseline) + assertEquals(periods, out) + } + + /** The pre-existing behaviour — a bare gap between two sleep runs — still merges. */ + @Test + fun `the original adjacent-pair merge still works`() { + val periods = listOf(sleep(0, 3000), sleep(3600, 9000)) + val out = SleepStager.bridgeSparseSleep(periods, sparse = true, hr = calmHr(0, 9000), baseline = baseline) + assertEquals(1, out.size) + assertEquals(9000L, out[0].end) + } + + /** + * The trace has to say WHY, and the blocking length is the number a reader needs. The old trace could + * only report runsBefore == runsAfter, which says the bridge did nothing and not what stopped it. + */ + @Test + fun `a blocked pair reports the bound that blocked it, with the active length`() { + val tooLong = (SleepStager.sparseBridgeActiveMaxMin * 60L) + 60L + val periods = listOf(sleep(0, 3000), active(3000, 3000 + tooLong), sleep(3000 + tooLong, 9000)) + val (_, attempts) = SleepStager.bridgeSparseSleepTraced( + periods, sparse = true, hr = calmHr(0, 9000), baseline = baseline, + ) + assertEquals(1, attempts.size) + assertEquals("activeTooLong", attempts[0].reason) + assertFalse(attempts[0].bridged) + assertEquals(SleepStager.sparseBridgeActiveMaxMin + 1L, attempts[0].activeMin) + } + + /** + * The tracer and the merge are ONE pass. Swift kept a shadow copy of the loop purely to trace it, + * which has to be edited in step with the real one — a trace that quietly disagrees with the + * behaviour it describes is worse than no trace at all. + */ + @Test + fun `the traced pass returns exactly what the plain one does`() { + val periods = listOf(sleep(0, 3000), active(3000, 3900), sleep(3900, 9000)) + val hr = calmHr(0, 9000) + assertEquals( + SleepStager.bridgeSparseSleep(periods, sparse = true, hr = hr, baseline = baseline), + SleepStager.bridgeSparseSleepTraced(periods, sparse = true, hr = hr, baseline = baseline).first, + ) + } + + /** + * #1657, the other half: hrSleepBandAcross judged on the MEAN, which a single arousal spike drags out + * of band — the exact statistic confirmSleepWithHR documents as wrong for this, and uses the median + * for instead. A sustained elevation must still be rejected, or the gate stops discriminating. + */ + @Test + fun `a brief spike no longer puts the whole interval out of band, a sustained one still does`() { + val spiky = calmHr(0, 3540) + calmHr(3541, 3660, bpm = 190) + assertTrue(SleepStager.hrSleepBandAcross(0, 3660, spiky, baseline)) + val sustained = calmHr(0, 3660, bpm = 110) + assertFalse(SleepStager.hrSleepBandAcross(0, 3660, sustained, baseline)) + } +} From e8700f24b050116c093b8431d76574d8c9ed01a3 Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Mon, 31 Aug 2026 13:48:17 +1200 Subject: [PATCH 2/4] sleep: the bound was too tight to reach the case it was written for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review, and the end-to-end test earned its place on the first run. The unit cases all passed with the bound at maxGapMin (20). A test through the real detectSleep pipeline did not: a FIFTEEN-minute interruption produced a TWENTY-ONE minute active run, so the bridge rejected it as activeTooLong and the night still came back as two sessions. The fix did not reach the case the issue is about, and nothing at the unit level could have shown that. The cause is structural rather than incidental. classifyStill smears the still/moving boundary by roughly its rolling window, and buildRuns closes runs at sample edges, so the DETECTED active run is systematically longer than the interruption it represents — about six minutes longer here. A bound reasoned from how long someone is actually out of bed will always be too tight, because that is not the quantity being measured. Bound is 30 now, and the doc says plainly that it is a judgement rather than dressing it as derived: a realistic trip of up to about a quarter of an hour, plus the ~6 minutes of smear that measurement showed, with headroom. Well under minSleepMin, because an interruption long enough to be a session in its own right is a genuine awakening and should split the night. The HR band remains the substantive guard, and the awake-interruption test still refuses to bridge at 110 bpm. Two end-to-end cases added, both through detectSleep on a sparse fixture with a real 15-minute up-and-about: one asserts the night survives as a single session, the other that an elevated-HR interruption does not. The reasoning that produced 20 was tidier than the reality; only the pipeline knew. Android 4,937 tests and StrandAnalytics 1,669 (one skip), zero failures on both. Doc lint and i18n clean. --- .../Sources/StrandAnalytics/SleepStager.swift | 26 ++++--- .../java/com/noop/analytics/SleepStager.kt | 26 ++++--- .../analytics/SleepStagerActiveBridgeTest.kt | 67 +++++++++++++++++++ 3 files changed, 103 insertions(+), 16 deletions(-) diff --git a/Packages/StrandAnalytics/Sources/StrandAnalytics/SleepStager.swift b/Packages/StrandAnalytics/Sources/StrandAnalytics/SleepStager.swift index a60a97f4fd..a38d70b5c0 100644 --- a/Packages/StrandAnalytics/Sources/StrandAnalytics/SleepStager.swift +++ b/Packages/StrandAnalytics/Sources/StrandAnalytics/SleepStager.swift @@ -225,15 +225,25 @@ public enum SleepStager { public static let sparseBridgeGapMin: Int = 90 /// A single intervening ACTIVE run up to this long may be absorbed when bridging two sleep runs - /// (#1657). Deliberately DEFINED as `maxGapMin` rather than given a number of its own: that is - /// already this file's threshold for "a discontinuity this long is decisive", and reusing it means - /// there is one such judgement in the detector instead of two that can drift apart. + /// (#1657). /// - /// The bound is the cheap half of the guard. The real one is the HR band across the whole span — a - /// genuine wake keeps HR elevated for its duration and fails it, while a brief stir does not. - /// `mergeMin` (15) already absorbs shorter active runs upstream in `mergePeriods`, so this covers the - /// band between that and a wake long enough to be a real break in the night. - public static let sparseBridgeActiveMaxMin: Int = maxGapMin + /// This started as `maxGapMin` (20), on the reasoning that the file already had a threshold for + /// "a discontinuity this long is decisive". An end-to-end test through `detectSleep` showed that was + /// too tight to reach the case the issue is about: a FIFTEEN-minute interruption produced a + /// TWENTY-ONE-minute active run, because `classifyStill` smears the still/moving boundary by roughly + /// its rolling window and `buildRuns` closes runs at sample edges. The detected run is systematically + /// longer than the interruption it represents, so a bound set from the interruption's true length + /// rejects it. + /// + /// 30 is a judgement, and stated as one rather than dressed up as derived: a realistic trip out of + /// bed of up to about a quarter of an hour, plus the ~6 minutes of smear that measurement showed, + /// with a little headroom. It is deliberately well under `minSleepMin` — an interruption long enough + /// to be a session in its own right is a genuine awakening and should split the night. + /// + /// The bound is the cheap half of the guard. The real one is the HR band across the whole span: a + /// wearer who is actually up keeps HR elevated for the duration and fails it, while a brief stir does + /// not. `mergeMin` (15) already absorbs shorter active runs upstream in `mergePeriods`. + public static let sparseBridgeActiveMaxMin: Int = 30 // MARK: - Stage 1–3 constants (sleep_features.py) diff --git a/android/app/src/main/java/com/noop/analytics/SleepStager.kt b/android/app/src/main/java/com/noop/analytics/SleepStager.kt index 9698ca03d2..7016c12c55 100644 --- a/android/app/src/main/java/com/noop/analytics/SleepStager.kt +++ b/android/app/src/main/java/com/noop/analytics/SleepStager.kt @@ -322,16 +322,26 @@ object SleepStager { /** * A single intervening ACTIVE run up to this long may be absorbed when bridging two sleep runs - * (#1657). Deliberately DEFINED as [maxGapMin] rather than given a number of its own: that is - * already this file's threshold for "a discontinuity this long is decisive", and reusing it means - * there is one such judgement in the detector instead of two that can drift apart. + * (#1657). * - * The bound is the cheap half of the guard. The real one is the HR band across the whole span — - * a genuine wake keeps HR elevated for its duration and fails it, while a brief stir does not. - * [mergeMin] (15) already absorbs shorter active runs upstream in [mergePeriods], so this covers - * the band between that and a wake long enough to be a real break in the night. + * This started as [maxGapMin] (20), on the reasoning that the file already had a threshold for + * "a discontinuity this long is decisive". An end-to-end test through [detectSleep] showed that was + * too tight to reach the case the issue is about: a FIFTEEN-minute interruption produced a + * TWENTY-ONE-minute active run, because [classifyStill] smears the still/moving boundary by roughly + * its rolling window and [buildRuns] closes runs at sample edges. The detected run is systematically + * longer than the interruption it represents, so a bound set from the interruption's true length + * rejects it. + * + * 30 is a judgement, and stated as one rather than dressed up as derived: a realistic trip out of + * bed of up to about a quarter of an hour, plus the ~6 minutes of smear that measurement showed, + * with a little headroom. It is deliberately well under [minSleepMin] — an interruption long enough + * to be a session in its own right is a genuine awakening and should split the night. + * + * The bound is the cheap half of the guard. The real one is the HR band across the whole span: a + * wearer who is actually up keeps HR elevated for the duration and fails it, while a brief stir does + * not. [mergeMin] (15) already absorbs shorter active runs upstream in [mergePeriods]. */ - const val sparseBridgeActiveMaxMin: Int = maxGapMin + const val sparseBridgeActiveMaxMin: Int = 30 // ── Stage 1–3 constants (sleep_features.py) ────────────────────────────── diff --git a/android/app/src/test/java/com/noop/analytics/SleepStagerActiveBridgeTest.kt b/android/app/src/test/java/com/noop/analytics/SleepStagerActiveBridgeTest.kt index 63a86106b8..255bd5af32 100644 --- a/android/app/src/test/java/com/noop/analytics/SleepStagerActiveBridgeTest.kt +++ b/android/app/src/test/java/com/noop/analytics/SleepStagerActiveBridgeTest.kt @@ -138,4 +138,71 @@ class SleepStagerActiveBridgeTest { val sustained = calmHr(0, 3660, bpm = 110) assertFalse(SleepStager.hrSleepBandAcross(0, 3660, sustained, baseline)) } + + // ── End to end, through detectSleep ────────────────────────────────────────────────────────── + // + // The unit cases above pin the bridge. These pin the thing the reporter actually saw: a night with + // one interruption in it arriving as ONE session instead of a truncated fragment. + + private val dev = "test" + /** 2025-06-10 00:00:00 UTC. */ + private val refMidnight = 1_749_513_600L + private fun at(hour: Int, min: Int = 0) = refMidnight + hour * 3_600L + min * 60L + + /** Still gravity at 1/min — constant orientation, so every delta is 0. */ + private fun still(from: Long, toExclusive: Long) = + (from until toExclusive step 60).map { + com.noop.data.GravitySample(deviceId = dev, ts = it, x = 0.0, y = 0.0, z = 1.0) + } + + /** Moving gravity at 1/min — orientation swings every sample, so deltas are large. */ + private fun moving(from: Long, toExclusive: Long) = + (from until toExclusive step 60).mapIndexed { i, _ -> i }.map { i -> + val t = from + i * 60L + if (i % 2 == 0) com.noop.data.GravitySample(deviceId = dev, ts = t, x = 1.0, y = 0.0, z = 0.0) + else com.noop.data.GravitySample(deviceId = dev, ts = t, x = 0.0, y = 1.0, z = 0.0) + } + + private fun hr1Hz(from: Long, toExclusive: Long, bpm: Int) = + (from until toExclusive).map { HrSample(deviceId = dev, ts = it, bpm = bpm) } + + /** + * A night with a 30-minute gravity dropout early (so the night reads sparse, as a 5/MG's does) and a + * 15-minute up-and-about in the middle. Before #1657 the active run made the two sleep halves + * unreachable to the bridge and each half faced the 60-minute floor alone. + */ + private fun interruptedNight(tripBpm: Int): Pair, List> { + val grav = still(at(0), at(0, 30)) + // 00:00-00:30 asleep + still(at(1), at(2)) + // 00:30-01:00 DROPOUT, then 01:00-02:00 asleep + moving(at(2), at(2, 15)) + // 02:00-02:15 up + still(at(2, 15), at(6)) // 02:15-06:00 asleep again + val hr = hr1Hz(at(0), at(2), 50) + + hr1Hz(at(2), at(2, 15), tripBpm) + + hr1Hz(at(2, 15), at(6), 50) + return hr to grav + } + + @Test + fun `a night with one short interruption is detected as a single session`() { + val (hr, grav) = interruptedNight(tripBpm = 52) + assertTrue("the fixture must read as sparse", SleepStager.isGravitySparse(grav, hr)) + val sessions = SleepStager.detectSleep(hr = hr, gravity = grav) + assertEquals("the interruption must not end the night", 1, sessions.size) + val spanMin = (sessions[0].end - sessions[0].start) / 60.0 + assertTrue("the whole night should survive, got $spanMin min", spanMin > 5 * 60) + } + + /** + * The same night with the wearer genuinely up — HR elevated for the whole quarter hour. The bridge + * must decline, because absorbing that would score wakefulness as sleep: wrong in a new direction and + * harder to notice than the truncation being fixed. + */ + @Test + fun `the same night with a genuinely awake interruption is not bridged into one`() { + val (hr, grav) = interruptedNight(tripBpm = 110) + val sessions = SleepStager.detectSleep(hr = hr, gravity = grav) + val spanMin = sessions.sumOf { (it.end - it.start) / 60.0 } + assertTrue("an awake interruption must not be absorbed (got ${sessions.size} sessions, $spanMin min)", + sessions.size != 1 || spanMin < 5 * 60) + } } From 40df49e26e0f1058908a3dd571a31fc43208b914 Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Mon, 31 Aug 2026 13:59:14 +1200 Subject: [PATCH 3/4] sleep: give Swift the end-to-end twin that found the bound wrong MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third review pass. The two detectSleep cases existed only in Kotlin, which is backwards: that pair is what caught the bound being too tight, and the reason it was too tight is a property of THIS pipeline — classifyStill smears the still/moving boundary and buildRuns closes at sample edges, so a 15-minute interruption becomes a 21-minute run. Leave the twin out and a divergence in Swift's windowing would make 30 silently wrong on Apple alone, with every unit case still green. They pass unchanged, so the smear is the same on both and the bound holds. Eleven cases each side now. Also worth recording what the disturbance path does with an absorbed interruption, since the reporter asked for exactly that ("a disturbance, not the end of the night"). A wake segment inside a session adds to WASO and increments `disturbances` (HypnogramMetrics), so a wake that IS staged is preserved rather than deleted. But both stagers call wake primarily off HR, not motion — the file says so at the motion-corroborated-wake note, and the motion-aware refinement is opt-in and only ever SHRINKS wake. So an interruption the bridge absorbed, which by construction had HR in the sleep band, will generally stage as light sleep rather than wake. The absorbed minutes do land in TST. That is the honest description and it is not the one given earlier in this branch: "staged awake, so no inflation" was wrong. The right framing is that the HR gate decides. Movement with sleep-band HR is a stir scored as sleep, which is defensible; movement with elevated HR never reaches the bridge at all. Trading four lost hours for fifteen generously-scored minutes is a far smaller error, and pointed the right way — but it is an error, not a clean win, and the next reader should not have to discover that from the code. StrandAnalytics 1,669 (one skip), zero failures. Doc lint clean. --- .../SleepStagerActiveBridgeTests.swift | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SleepStagerActiveBridgeTests.swift b/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SleepStagerActiveBridgeTests.swift index 4649693669..48c398ad18 100644 --- a/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SleepStagerActiveBridgeTests.swift +++ b/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SleepStagerActiveBridgeTests.swift @@ -120,4 +120,69 @@ final class SleepStagerActiveBridgeTests: XCTestCase { XCTAssertFalse(SleepStager.hrSleepBandAcross(0, 3660, hr: calmHr(0, 3660, bpm: 110), baseline: baseline)) } + + // MARK: - End to end, through detectSleep + // + // The unit cases above pin the bridge. These pin the thing the reporter actually saw: a night with + // one interruption arriving as ONE session instead of a truncated fragment. + // + // This twin is not optional. The Kotlin version of exactly this test is what found the bound wrong — + // every unit case passed at 20 while the real pipeline still returned two sessions, because + // classifyStill smears the still/moving boundary and buildRuns closes at sample edges, so a 15-minute + // interruption becomes a 21-minute run. That smear is a property of THIS implementation, so without + // the twin a divergence in Swift's windowing would leave the bound silently wrong on Apple only. + + /// 2025-06-10 00:00:00 UTC. + private var refMidnight: Int { 1_749_513_600 } + private func at(_ hour: Int, _ minute: Int = 0) -> Int { refMidnight + hour * 3_600 + minute * 60 } + + /// Still gravity at 1/min — constant orientation, so every delta is 0. + private func stillG(_ from: Int, _ toExclusive: Int) -> [GravitySample] { + stride(from: from, to: toExclusive, by: 60).map { GravitySample(ts: $0, x: 0, y: 0, z: 1.0) } + } + + /// Moving gravity at 1/min — orientation swings every sample, so deltas are large. + private func movingG(_ from: Int, _ toExclusive: Int) -> [GravitySample] { + stride(from: from, to: toExclusive, by: 60).enumerated().map { i, t in + i % 2 == 0 ? GravitySample(ts: t, x: 1.0, y: 0, z: 0) + : GravitySample(ts: t, x: 0, y: 1.0, z: 0) + } + } + + private func hr1Hz(_ from: Int, _ toExclusive: Int, _ bpm: Int) -> [HRSample] { + (from ..< toExclusive).map { HRSample(ts: $0, bpm: bpm) } + } + + /// A night with a 30-minute gravity dropout early (so it reads sparse, as a 5/MG's does) and a + /// 15-minute up-and-about in the middle. + private func interruptedNight(tripBpm: Int) -> ([HRSample], [GravitySample]) { + let grav = stillG(at(0), at(0, 30)) + + stillG(at(1), at(2)) + + movingG(at(2), at(2, 15)) + + stillG(at(2, 15), at(6)) + let hr = hr1Hz(at(0), at(2), 50) + + hr1Hz(at(2), at(2, 15), tripBpm) + + hr1Hz(at(2, 15), at(6), 50) + return (hr, grav) + } + + func testANightWithOneShortInterruptionIsDetectedAsASingleSession() { + let (hr, grav) = interruptedNight(tripBpm: 52) + XCTAssertTrue(SleepStager.isGravitySparse(grav, hr: hr), "the fixture must read as sparse") + let sessions = SleepStager.detectSleep(hr: hr, gravity: grav) + XCTAssertEqual(sessions.count, 1, "the interruption must not end the night") + let spanMin = Double((sessions[0].end - sessions[0].start)) / 60.0 + XCTAssertGreaterThan(spanMin, 5 * 60, "the whole night should survive, got \(spanMin) min") + } + + /// The same night with the wearer genuinely up — HR elevated for the whole quarter hour. Absorbing + /// that would score wakefulness as sleep: wrong in a new direction and harder to notice than the + /// truncation being fixed. + func testTheSameNightWithAGenuinelyAwakeInterruptionIsNotBridgedIntoOne() { + let (hr, grav) = interruptedNight(tripBpm: 110) + let sessions = SleepStager.detectSleep(hr: hr, gravity: grav) + let spanMin = sessions.reduce(0.0) { $0 + Double($1.end - $1.start) / 60.0 } + XCTAssertTrue(sessions.count != 1 || spanMin < 5 * 60, + "an awake interruption must not be absorbed (\(sessions.count) sessions, \(spanMin) min)") + } } From 4028f75920442eb6b427c23a36761e52b4abc8d7 Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Mon, 31 Aug 2026 14:17:13 +1200 Subject: [PATCH 4/4] sleep: pin the new trace line's exact rendering on both platforms Parity re-review. Everything lines up - constants both 30, reason tokens and their ORDER identical, gate names identical, hrSleepBandAcross down to one definition and two call sites each side (Swift correctly lost its third when the shadow tracer went), and the eleven test cases map one-to-one with nothing one-sided. The three emitted trace lines are byte-identical too, after normalising the interpolation syntax. But that was checked BY HAND, and nothing in the tree keeps it true: Tools/parity_cases holds one unrelated fixture, and neither suite asserted the rendered line at all - only the fields behind it. A format change on one platform would have diverged in silence and surfaced when somebody tried to read a capture from the other. Both suites now assert the same whole rendered string, byte for byte, which is the idiom this repo already uses elsewhere for exactly this reason. It is not a real cross-language oracle - nothing compares the two automatically - but it converts a silent divergence into a failing test on whichever side moved, and a reviewer looking at one literal can see the other. Android 4,938 and StrandAnalytics 1,672 (one skip), zero failures. Doc lint and i18n clean. --- .../SleepStagerActiveBridgeTests.swift | 15 +++++++++++++ .../analytics/SleepStagerActiveBridgeTest.kt | 22 +++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SleepStagerActiveBridgeTests.swift b/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SleepStagerActiveBridgeTests.swift index 48c398ad18..561e177b7f 100644 --- a/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SleepStagerActiveBridgeTests.swift +++ b/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SleepStagerActiveBridgeTests.swift @@ -185,4 +185,19 @@ final class SleepStagerActiveBridgeTests: XCTestCase { XCTAssertTrue(sessions.count != 1 || spanMin < 5 * 60, "an awake interruption must not be absorbed (\(sessions.count) sessions, \(spanMin) min)") } + + /// The RENDERED line, not just the fields — and deliberately the whole string, byte for byte. + /// + /// The Kotlin twin asserts this identical literal. Nothing in the tree compares the two languages' + /// trace output automatically, so a format change on one side would otherwise diverge in silence and + /// only surface when someone tried to read a capture from the other platform. + func testThePairLineRendersExactlyAsItsKotlinTwinDoes() { + let line = SleepStager.GateTrace.runLine( + index: -1, startTs: 0, endTs: 0, verdict: SleepStager.GateVerdict.dropped, gate: "sparseBridgePair", + detail: "pair=0 gapMin=23 activeMin=21 hrInSleepBand=true reason=activeTooLong") + XCTAssertEqual( + line, + "gate run=-1 spanS=0 DROPPED gate=sparseBridgePair " + + "pair=0 gapMin=23 activeMin=21 hrInSleepBand=true reason=activeTooLong") + } } diff --git a/android/app/src/test/java/com/noop/analytics/SleepStagerActiveBridgeTest.kt b/android/app/src/test/java/com/noop/analytics/SleepStagerActiveBridgeTest.kt index 255bd5af32..e4bacdfe5d 100644 --- a/android/app/src/test/java/com/noop/analytics/SleepStagerActiveBridgeTest.kt +++ b/android/app/src/test/java/com/noop/analytics/SleepStagerActiveBridgeTest.kt @@ -205,4 +205,26 @@ class SleepStagerActiveBridgeTest { assertTrue("an awake interruption must not be absorbed (got ${sessions.size} sessions, $spanMin min)", sessions.size != 1 || spanMin < 5 * 60) } + + /** + * The RENDERED line, not just the fields — and deliberately the whole string, byte for byte. + * + * The Swift twin asserts this identical literal. Nothing in the tree compares the two languages' + * trace output automatically (Tools/parity_cases holds one unrelated fixture), so a format change on + * one side would otherwise diverge in silence and only surface when someone tried to read a capture + * from the other platform. Pinning the same literal both sides turns that into a failing test on + * whichever side moved. + */ + @Test + fun `the pair line renders exactly as its Swift twin does`() { + val line = SleepStagerTrace.runLine( + -1, 0, 0, SleepStagerTrace.Verdict.DROPPED, "sparseBridgePair", + "pair=0 gapMin=23 activeMin=21 hrInSleepBand=true reason=activeTooLong", + ) + assertEquals( + "gate run=-1 spanS=0 DROPPED gate=sparseBridgePair " + + "pair=0 gapMin=23 activeMin=21 hrInSleepBand=true reason=activeTooLong", + line, + ) + } }