Repository navigation
Close the final HR/HRV window on the shipped SleepStager path, and drop the unused RecoveryScorer helpers - #1936
Merged
Merged
Conversation
…ed path SleepStager.sessionRestingHR and SleepStager.sessionHrvWindows prefilter a closed [start, end] window and then bin it as [t, t + 300) with a loop that stops at `end` — the same defect RecoveryScorer.restingHR carried before ab14a95. A sample or beat sitting exactly on an ALIGNED `end` passes the prefilter and then belongs to no bin: admitted as data, silently ignored. Unlike the RecoveryScorer helpers, these two are the SHIPPED path. Both platforms compute the per-session resting HR and avgHRV through them (SleepStager's own session loops and AnalyticsEngine's fallback), and sessionHrvWindows also feeds the HRV nightly trace and the last-deep-run comparator. So this moves a displayed number, in the aligned-endpoint case only. The final bin now closes on `end` on both platforms ([t, t + 300) for every bin except the last, which is [t, end]); a zero-length window collapses to that one closed bin, so a session whose beats all sit on `start == end` yields a window instead of none. Window startTs, the stage-tagging center and the RMSSD math are unchanged. Tests are red first, then green: the four aligned cases fail on the unmodified source (Swift: resting floor reads 70 instead of 40, the sweep disagrees at end=600 and end=900, the final HRV window comes back empty, zero-length avgHRV is nil; Kotlin: the same four). Each is paired with a non-aligned twin that passes before and after, so the change cannot be mistaken for one to trailing partial bins. The Kotlin literals are the verbatim stdout of a standalone Swift oracle over the fixed binning, and SleepStagerTests.swift pins the same literals so the oracle guards both directions. The HRV cases use a constant 900 ms RR series whose RMSSD is analytically 0.0 on both platforms, so they pin the BINNING and not the RMSSD math. Verification: swift test --filter SleepStagerTests (84 passed) and the wider SleepStager/AnalyticsEngine/SleepSession selection (221 passed); Gradle testFullDebugUnitTest for com.noop.analytics (2019 passed, including the 8 new cases). No strap and no app-target build is involved — this is pure package math.
RecoveryScorer.restingHR and RecoveryScorer.recoveryIndexSlope have no caller outside their own tests on either platform. The shipped resting HR and HRV come from SleepStager.sessionRestingHR / sessionHrvWindows (via SleepStager's session loops and AnalyticsEngine's fallback), which now carry the closed-window endpoint rule themselves. recovery(...) takes the resting HR and the Recovery-Index slope as caller-supplied parameters and never called these two. Confirmed before removing, over non-test sources only (Strand/, StrandiOS*/, Packages/*/Sources, Tools/, android/app/src/main, docs/, project.yml): no production call site, no public consumer in the app targets, and the only prose references were the doc/API mentions updated here. Both same-named `recoveryIndexSlope` PARAMETERS of recovery(...) and its chargeDrivers overload stay — they are the scorer's inputs, not this helper — and their doc comments no longer point at the removed function. Removed with them: restingHRWindowS, restingHRMinBinSamples, restingHRMinPlausibleBpm and recoveryIndexMinBins on both platforms. They were referenced only from inside the two deleted functions, and their doc comments describe an artifact-hardening rule (ryanbr#686) and a bin gate that no longer exist anywhere in the tree — leaving them would leave documentation for code that is gone. Nothing else in either platform reads them. Tests deleted with the code they covered: the restingHR floor/ryanbr#686 cases and the recoveryIndexSlope cases in RecoveryScorerTests.swift, RecoveryScorerRestingHrTest.kt and RecoveryScorerWindowEndpointTest.kt (both whole files), and the three slope cases plus the now-unused hr/slopeSeries fixtures in RecoveryIndexActivityBalanceTest.kt. The recovery(...) parameter cases in that file and in RecoveryScorerTests.swift are untouched: they pin the scorer terms, which still exist. docs/ANALYTICS.md now documents the endpoint rule where the shipped code is — a new "Per-session resting HR and HRV" section under SleepStager — and the RecoveryScorer section states that recovery(...) takes resting HR as an input rather than computing one. docs/LIBRARY.md drops restingHR(_:start:end:) from the RecoveryScorer public-API row. Verification: swift test --filter '(RecoveryScorerTests|SleepStagerTests)' (97 passed); Gradle testFullDebugUnitTest for com.noop.analytics (2001 passed, 18 fewer than before, exactly the removed cases); python3 Tools/doc_comment_lint.py (OK, 24 baselined sites). No behaviour change: nothing that runs called these.
… rule can move a shipped number
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.
Follow-up to #1927. That PR closed the final five-minute bin on the window end in
RecoveryScorer.restingHR/recoveryIndexSlope— but those two helpers have no production caller. The shipped per-session resting HR and HRV windows come fromSleepStager.sessionRestingHRandSleepStager.sessionHrvWindowson both platforms, and they carried the same defect: a closed[start, end]prefilter over[t, t + 300)bins with a loop that stops atend, so a sample or beat sitting exactly on an alignedendis admitted and then lands in no bin.Part 1 — the same rule on the shipped path. The final bin now closes on
endon both platforms ([t, t + 300)for every bin except the last, which is[t, end]; a zero-length window is that single closed bin). WindowstartTs, the stage-tagging centre and the RMSSD math are unchanged. Because these two functions are shipped, this can move a displayed number on a night whose session length is an exact multiple of 300 s with a sample onend: per-sessionrestingHR/avgHRVand the HRV trace. Stored rows keep the old value until that night is re-scored; no migration.Paired aligned/non-aligned endpoint tests on both platforms (8 Swift, 8 Kotlin), red before the fix on the same four cases on each side, green after. The 37-value
sessionRestingHRsweep is one oracle literal asserted verbatim by both tests. Wider runs unchanged:swift testover SleepStager/AnalyticsEngine/SleepSession (221 passed) and thecom.noop.analyticsKotlin package (2019 tests).Part 2 — dead code out.
RecoveryScorer.restingHRandRecoveryScorer.recoveryIndexSlopeare removed on both platforms together with their tests and the four constants only they used (restingHRWindowS,restingHRMinBinSamples,restingHRMinPlausibleBpm,recoveryIndexMinBins), after a repo-wide caller grep found no production or public consumer. TherecoveryIndexSlopeparameter ofrecovery(...)stays (a caller-supplied scorer input; today only tests supply it).docs/ANALYTICS.mdnow documents the endpoint rule where the shipped code is;docs/LIBRARY.mdno longer lists the removed helper. The dated plandocs/superpowers/plans/2026-06-29-oura-local-ble-architecture.md(lines 15 and 61) still names the old helper and was left as history.Two things a maintainer may want to weigh: the shipped-number change above, and the constant removal — leaving
restingHRMinPlausibleBpmbehind would document an artifact gate no code applies.Observed, not fixed here: the shipped
sessionRestingHRhas no #686-style artifact gate (no minimum bin sample count, no plausibility floor); a single-beat bin can win the night's floor. Pre-existing on both platforms; worth its own issue.Limits: Linux host, no Xcode — app targets were not built (
app-build.ymlis the real check); no full suite on either platform, selection by affected scope. Synthetic fixtures only.