Skip to content

fix(hrv): stop reporting SDNN for a capture already known to be over-counted - #1085

Merged
ryanbr merged 1 commit into
ryanbr:mainfrom
pipiche38:fix/gate-sdnn-on-overcounted-rr
Aug 5, 2026
Merged

fix(hrv): stop reporting SDNN for a capture already known to be over-counted#1085
ryanbr merged 1 commit into
ryanbr:mainfrom
pipiche38:fix/gate-sdnn-on-overcounted-rr

Conversation

@pipiche38

Copy link
Copy Markdown

Branch: fix/gate-sdnn-on-overcounted-rr (based on main @ 7c7b3f0c, commit a863170f)
Base: main. No migration, no stored-value change, no new UI strings.
Related: follows #550 (which added the verdict) · #1072 / #1082 (the emission-order fix this
deliberately does not pre-empt) · #882/#883 drew the same boundary for respiration.


The gap

HRVAnalyzer.classifyCoverage (#550) already decides whether a window's beat-time fits the wall clock
it spans, and the nightly hrv diag line has printed the answer ever since:

hrv diag day=2026-08-05 rmssd=85ms sdnn=197ms meanNN=1108ms rr=36225/31637 rejected=13%
         coverage=1.33 collapsedCov=0.87 dupBeats=0 rrIntegrity=sameSecondOverCount

Nothing acted on it. The app had diagnosed the capture as holding some beats twice and printed an
SDNN anyway
, next to the field saying it could not be trusted.

SDNN is the standard deviation of every NN interval in the window, so duplicated beats inflate it
directly. On a ring whose banked R-R covers 1.25× the wall clock it spans, a sleeping night reads
197 ms against a 40–100 ms physiological range. That is not a small bias — it describes a heart
that does not exist.

The change

One named boundary, HRVAnalyzer.beatSpreadIsTrustworthy(_:) (Kotlin twin
HrvAnalyzer.beatSpreadIsTrustworthy), and two places that act on it:

  1. The nightly hrv diag line withholds the value — sdnn=withheld — rather than printing it.
    The rrIntegrity= field on the same line already says why, so the log stays self-diagnosing.
  2. RhythmScreener.screenWindow returns unreadable for an over-counted window instead of
    describing it. Every statistic there is a spread over the interval cloud — SD2 is built from SDNN
    outright — so an over-count makes a rhythm read more varied than it was. That is the wrong
    direction to be wrong in for a label a person reads.

What is not gated, and why

Honest scope

The rhythm screener is not wired to any UI yet (its own header notes the screening verdict is
held behind a separate go/no-go). So today's only live effect is the diagnostic line. The screener
change is preventive: it makes it impossible for that engine to ship a wrong label later, at the cost
of four lines.

Known gap, deliberately left: SleepStager computes a per-epoch sdnn feature over 30 s windows
on both platforms. Gating that needs a per-epoch coverage measure, which is a different measurement
from the night-level one, and the stager is already unsettled (#930, #1008). Out of scope here rather
than done badly.

Tests

Package-level on both platforms (swift-packages.yml / ./gradlew testFullDebugUnitTest):

  • the gate refuses both over-count verdicts and keeps the other three;
  • end-to-end on the shape that motivated it — 60 beats delivered as 10 records of 6, each record
    stamping all six at its own second, records 5 s apart (60 s of beat-time in a 45 s span) — refuses,
    while the same beats stamped one per second classify plausible and stay readable;
  • RhythmScreener: a banked window reads unreadable with sd2 == nil; the same beats honestly
    stamped still read steady; a window with no timestamps behaves exactly as before.

Every one has a Kotlin twin.

Verification

  • swift test StrandAnalytics: 1241 pass, 0 failures.
  • ./gradlew testFullDebugUnitTest: 3,474 tests, the same 3 pre-existing locale failures as clean
    main (AiCoachContextTest, StandardHrSensorFormatTest ×2).
  • App-target Swift is touched (IntelligenceEngine.swift) and no default CI builds it
    (app-build.yml is disabled): xcodebuild Strand (macOS) and NOOPiOS both BUILD SUCCEEDED.
  • No hardware needed — this is pure analytics plus one log line. Nothing on the BLE path changed.

…counted

`classifyCoverage` (ryanbr#550) works out whether a window's beat-time fits the wall
clock it spans, and the nightly `hrv diag` line has printed the verdict since.
Nothing ever acted on it. So a capture the app had already diagnosed as holding
some beats twice still produced an SDNN, and printed it next to the verdict
saying it could not be trusted.

SDNN is the spread over EVERY interval in the window, so duplicated beats
inflate it directly. On a ring whose banked R-R covers 1.25x the wall clock it
spans, a sleeping night reads ~197 ms against a 40-100 ms physiological range.
The number is not a little off; it describes a heart that does not exist.

`HRVAnalyzer.beatSpreadIsTrustworthy(_:)` names the boundary once, and two
places act on it:

- the nightly `hrv diag` line withholds SDNN (`sdnn=withheld`) rather than
  printing it; the `rrIntegrity=` field on the same line already says why;
- `RhythmScreener.screenWindow` returns `unreadable` for an over-counted window
  instead of describing it. Every statistic there is a spread over the interval
  cloud — SD2 is built from SDNN outright — so an over-count makes a rhythm read
  MORE varied than it was, which is the wrong direction to be wrong in for a
  label a person reads. That engine is not wired to any UI yet; this makes sure
  it cannot ship a wrong label when it is.

Only the two OVER-COUNT verdicts gate. `underCovered` and `unmeasurable` stay
trusted: neither duplicates a beat, and `unmeasurable` is exactly what a live
spot reading looks like — real-time beats carrying no timestamps to measure
coverage with. Gating those would refuse honest readings, the opposite of the
point. The HRV snapshot screen is therefore untouched.

RMSSD and pNN50 are deliberately NOT gated. Their dominant error on a banked
stream was the lost within-second emission order (ryanbr#823, root-caused in ryanbr#1072),
which is fixed at the write path; whether they need a gate of their own is a
question for a post-fix capture, not an assumption to bake in now.

Not device-specific: a WHOOP night that ever measured an over-count would be
gated on the same evidence, and no code here asks which strap produced the beats.

Verification: `swift test` StrandAnalytics 1241 pass, incl. new tests that the
gate refuses the two over-count verdicts, keeps the other three, and holds
end-to-end on banked bursts (60 beats as 10 records of 6, records 5 s apart)
while the same beats stamped one per second stay readable. Kotlin twins of every
one of those. `./gradlew testFullDebugUnitTest` 3,474 tests with the same 3
pre-existing locale failures as clean main. App targets built locally since no
default CI compiles them: `xcodebuild` Strand (macOS) and NOOPiOS both succeed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AiQZ5XBeAMgnyUugPMqXPi
@pipiche38
pipiche38 marked this pull request as ready for review August 5, 2026 09:35
@ryanbr

ryanbr commented Aug 5, 2026

Copy link
Copy Markdown
Owner

Reviewed and merging — the beatSpreadIsTrustworthy gate is a clean byte-parity twin, the hrv diag change is log-only (the stored nightly metric is avgHrv/RMSSD, not SDNN, so nothing persisted or scored changes), and the live HRV snapshot is correctly left trusted (unmeasurable). Tests are proper twins and all CI is green.

One correction to the "honest scope" note for the record: on macOS/iOS the RhythmScreener gate does have a live effect. The shipping RhythmView (reachable via RootView) builds windows in V5PillarHosts through WindowInput(rr:motionStill:), and that initializer populates ts (ts: rr.map { $0.ts }) — so Gate 4's ts-present condition is satisfied and the gate fires. Over-counted windows now read .unreadable on the Rhythm screen rather than a label + SD2.

That's benign and arguably the correct outcome — .unreadable is an already-handled window state (the sparse/motion gates produce it too), so it degrades gracefully with no blank/broken view — but it's a real, user-visible change for exactly the over-counting captures this targets (an Oura banked night could read mostly unreadable). Worth an eyeball on how an all-over-counted night renders in RhythmView, and worth not describing it as diagnostic-only. (Android's RhythmScreen is still a stub — night = null, windows = emptyList() — so no effect there; the diag-line fix is the part in parity.)

Merging as-is since the behavior is correct; just flagging so the record is accurate.

@ryanbr
ryanbr merged commit a49f35e into ryanbr:main Aug 5, 2026
16 checks passed
@pipiche38
pipiche38 deleted the fix/gate-sdnn-on-overcounted-rr branch August 5, 2026 11:37
@pipiche38

Copy link
Copy Markdown
Author

Hardware read on the gate, from an overnight drain on a build carrying it
(integration/oura-full@692f24a1, Gen-3 ring, night 2026-08-05 22:12 → 2026-08-06 07:18).

The gate works in both directions. Same log, consecutive nights, nothing else changed:

hrv diag day=2026-08-06 rmssd=70ms sdnn=174ms      meanNN=1080ms coverage=1.03 rrIntegrity=plausible
hrv diag day=2026-08-05 rmssd=85ms sdnn=withheld   meanNN=1108ms coverage=1.33 rrIntegrity=sameSecondOverCount
hrv diag day=2026-08-04 rmssd=68ms sdnn=withheld   meanNN=1041ms coverage=2.21 rrIntegrity=crossSecondOverCount
hrv diag day=2026-08-01 rmssd=56ms sdnn=130ms      meanNN=1020ms coverage=0.71 rrIntegrity=underCovered

Every over-counted night withholds; the plausible night prints a number; underCovered is not
gated and still prints, as designed. rmssd and meanNN are untouched throughout. That is exactly
the specified behaviour, on real data.

But the honest reading is that the gate is necessary and not sufficient. On the night it passes,
SDNN reads 174 ms — three to four times a physiological overnight value, and the user sees it.

The over-count screen is not the thing keeping it high. This night is not over-counted by any
measure: coverage 1.03, 0x60 records tile the timeline at a fill ratio of 0.990, and 0x44 is
confirmed absent from the wire (0 of 29,597 raw records). Two independent checks say the residual is
in the beat values, not in beat duplication:

  • 5-minute windowed SDNN, median 192 ms (n = 106 windows). Slow overnight drift would leave the
    windows small; it does not. The spread is inside the windows.
  • Record-medians-only series — one robust value per record, immune to both duplication and
    within-record noise — still gives SDNN 185 ms.

Within a record, the six beats have a median spread of 140 ms but a p90 of 656 ms and a max of
1,648 ms; ~6 % of beats sit outside ±25 % of their own record's median, and 4.1 % of the night's
beats imply >100 bpm during sleep. Between records the trend is clean (consecutive record medians
differ by a median of 48 ms). So a minority of banked beats are not beat-accurate, scattered through
otherwise-good records — the same premise #883's respiration gate rests on.

Dropping that tail moves whole-night SDNN 224 → 186 ms in offline arithmetic, so even a perfect
de-bank does not land SDNN in a physiological band. The conclusion is that SDNN over banked Oura
IBI is not recoverable by de-duplication
, and this PR's boundary is the right one — it just needs
to be reached more often than rrIntegrity alone reaches it.

Suggested follow-up (not filed): widen the refusal from "over-counted" to "not beat-accurate",
e.g. also withhold when the within-record beat spread exceeds a threshold. That would have caught
this night. Happy to draft it separately rather than reopening this PR.

On the merge-comment correction — still owed. The RhythmView eyeball has not been done: the
only screen captured this drain was Sleep. The log shows the gate firing for 08-05 and earlier, so
those nights should render as .unreadable on the Rhythm screen, but that is inference from the
diagnostics, not an observation. Carried forward to the next capture.

ryanbr pushed a commit that referenced this pull request Aug 6, 2026
… is over-counted (#1108)

#1085 taught the app to refuse SDNN on an over-counted capture. A drain the night
after it merged showed that gate is necessary but not sufficient: the 2026-08-06
Oura night measured coverage 1.03, `rrIntegrity=plausible` — no duplication at all,
its records tiling the timeline at a fill ratio of 0.990 — and still printed
**SDNN 174 ms** against a 40-100 ms physiological range.

Over-counting was never what made that number wrong. A BANKED stream stamps a whole
record of intervals on one coarse timestamp, so the stored values are a decomposition
of a record period rather than beat-to-beat measurements. The per-record SUM is right
to ~1%, which is why meanNN and resting HR stay correct and WHOOP-validated, while the
individual intervals are not. Coverage cannot see that fault, by construction: it
compares beat-time against wall-clock, and a banked night can be textbook.

Measured on that night, after the shipped Malik ectopic filter:
  - within-5-minute SDNN 123 ms, against 30-80 ms physiological;
  - only 94 ms of the whole-night figure is genuine trend (HR really moves 47-86 bpm);
  - widening the ectopic window does not reach it — radius 2 -> 20 moves the
    within-window figure just 124 -> 99 ms. Each interval sits within 20% of its own
    local median, so no per-beat artifact rule can see this: the fault is in the
    decomposition, not in outliers.

So the gate has to be on the stream's nature, not on a spread statistic:

  - `beatAccurateFraction(tsSec:rrMs:)` — the fraction of consecutive beats whose
    wall-clock gap matches their own R-R value. Beat-accurate streams step one
    interval per beat and measure ~1.0; a banked stream's gaps are 0 s against ~1 s
    values and it collapses toward 0.
  - `beatValuesAreTrustworthy(beatAccurateFraction:)` — SDNN is withheld below the
    boundary. Independent of `beatSpreadIsTrustworthy`; neither implies the other, and
    both now gate.

The boundary is not tuned. The two populations do not overlap near it: a beat-accurate
stream measures ~100%, and every banked Oura overnight measured to date sits at
**2.6-6.6%** (five nights, 2026-07-29 -> 08-06). RMSSD/pNN50 stay ungated for the same
reason they are ungated by the coverage verdict. `unmeasurable` live spot readings are
untouched: too-short or mismatched input returns 1.0 and stays trusted, so an honest
live capture is never suppressed.

The constants duplicate the ones the respiration gate uses for the same judgement
(#882/#883) rather than sharing them — that gate lives in `SleepStager` on a branch
that is not upstream. If it lands, the two should collapse onto this definition; the
boundary is worth drawing once, in one place, for both.

`hrv diag` now carries `beatAccurate=` so the distribution can be gathered from traces
that already exist, the same way `coverage=` was added before acting on it.

Verification: `swift test` StrandAnalytics 1248, incl. 6 new tests — the decisive one
pins a PERFECTLY COVERED banked night (coverage 1.0, verdict plausible) passing the
over-count gate and being refused by the new one, which is the case that motivated this.
Kotlin twins of each in `HrvRrCoverageTest`. `./gradlew testFullDebugUnitTest` 3,516
tests with the same 3 pre-existing locale failures as clean main (verified on a clean
worktree, not assumed). `Strand` (macOS) built locally since `IntelligenceEngine.swift`
is app-target Swift that no default CI job compiles.


Claude-Session: https://claude.ai/code/session_01Kyxz22d4v7QHWFCSWTvdJq

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants