Repository navigation
fix(hrv): read same-second R-R beats in emission order, not magnitude order - #830
Conversation
… order (#823) Both platforms read a second's R-R intervals sorted by VALUE (ORDER BY ts, rrMs). Sorting makes adjacent values similar by construction, and RMSSD is built entirely from successive differences, so this biased a headline metric one-directionally down. On the issue's five-beat example: 34.85 ms in emission order, 12.72 ms sorted — a -22.14 ms error. Measured at ~1.1% of same-second groups on 5/MG. Adds `ord`, the beat's position among all beats sharing its `ts`, stamped at decode time and used to lead the read sort. Why not the two-line fix. tanarchytan's ORDER BY ts, seq treats `seq` as an insertion counter. It is not: assignRrSeq keys on (ts, rrMs), so seq counts repeats of an IDENTICAL beat and every distinct beat in a second carries 0. They would all tie and the order would become whatever the scan returns — swapping a deterministic wrong order for an undefined one, so RMSSD could vary between reads of the same data. Why ord is not in the key. The key stays (deviceId, ts, rrMs, seq). A per-second insertion counter in the key restarts per batch and collides distinct beats arriving in separate flushes — the data-loss regression assignRrSeq's own comment warns about. `ord` never decides which rows survive; it only informs read order. Legacy rows. Pre-migration rows keep ord NULL: the order was never recorded, so it cannot be backfilled and a guess would assert something nobody observed. SQLite sorts NULL first in ASC, so an all-legacy second ties on ord and falls through to the old (rrMs, seq) order — existing data reads back exactly as before, no COALESCE and no sentinel. Both stores are SQLite, so this matches on Room and GRDB. Parity: Room v23->24 and GRDB v30-rr-ord, both additive nullable ALTERs, no table rebuild, no row touched. The ORDER BY clauses were extracted from both sources and compared token-by-token, then executed against real SQLite: identical. Also fixes the raw-CSV export, which ordered by ts alone and so left same-second beats UNDEFINED — identical data could export two different ways. Also corrects the RrInterval doc, which said the Swift key widening was still pending. It had already shipped as v24-rr-seq. Swift tests cover this end-to-end and CI runs them: emission order restored, RMSSD back to 34.85, legacy NULL rows unchanged, mixed seconds deterministic across repeat reads, and ord absent from the primary key. Kotlin tests cover the migration shape and the stamping; there is no SQLite driver on the JVM unit-test classpath, and android.yml is disabled, so the Kotlin half is unrun.
XCTAssertEqual takes autoclosures, which cannot contain await. Bind the result to a let first. Compile error only, no assertion changed.
… undone (#823) Every analytics entry point re-sorts R-R by ts before computing RMSSD — deliberately, "the table read is already ordered, but we don't assume it". Swift's sorted(by:) is explicitly documented as NOT stable, so that defensive re-sort was free to re-scramble the same-second beats the store fix just put in emission order, silently reintroducing the bias at the point of use. It is stable in practice today (timsort since Swift 5), which is exactly why this would not have shown up in testing — a headline metric resting on an unguaranteed stdlib implementation detail. Adds Array<RRInterval>.sortedByTsStable(), decorating with the original offset so the comparator is TOTAL and no two elements compare equal. Stability becomes a property of our code. Applied at all seven R-R ts-sorts: HRVAnalyzer.rollingRmssd, HRVFreqDomain, SleepStager, SleepStagerV2, AnalyticsEngine (x2), ShortcutHealthExport. Kotlin needs no twin: sortedBy delegates to java.util.Arrays.sort, stable by contract, and HrvAnalyzer already stated the requirement ("Stable on equal ts"). Swift now matches that on paper as well as in behaviour. Other ts-sorts in StrandAnalytics (hr, gravity, steps) are untouched: those streams carry one sample per second, so there are no ties for stability to preserve.
|
Re-review found a defect that made the store fix ineffective on iOS. Pushed. Every analytics entry point re-sorts R-R by It is stable in practice today (timsort since Swift 5), which is precisely why this would not have surfaced in testing. A headline metric resting on an unguaranteed stdlib implementation detail. The asymmetry is the tell: Kotlin already stated the requirement — Added Left alone: the hr/gravity/steps All 9 jobs green, including the new |
Missed in the previous pass because the sort is chained across lines
(rr.filter{}\n.sorted{}), so a receiver-anchored grep did not see it. Found by
scanning every .sorted by its ENCLOSING function signature instead.
It matters more than most: step 2 reconstructs beat times by cumulative sum, so the
order of a second's beats shifts every subsequent beat time and with it the RSA
respiration estimate. Kotlin's twin already used sortedBy, stable by contract, so
this was also a live parity divergence.
Verified by signature scan that this was the last one: every other ts-sort inside an
R-R-taking function is on hr/gravity/resp, which carry one sample per second and so
have no ties for stability to preserve.
Same defect class, found by the signature scan: scorePace sorts sample.rr by ts and feeds it to cleanRR -> RMSSD, so same-second order changes the score. Kotlin's twin already used sortedBy, stable by contract — another live parity divergence. NOT part of #823's store-read chain: RrBeat comes from the live paced session, not from rrIntervals(). Fixed here because it is the same bug, not because the store fix needs it. Spelled out locally rather than reusing WhoopProtocol's helper. ResonanceEngine.swift imports only Foundation and documents itself as PURE + DB-free, "decoupled from the storage entities so the engine takes pure inputs"; adding a module dependency for one comparator would break that on purpose. I first generalised the helper behind a TimestampedBeat protocol to share it, then dropped that — one conformer did not justify the indirection, and the second user could not import it anyway.
|
Third pass. My previous sweep was receiver-anchored ( 1. 2. The scan confirms these were the last: every other ts-sort inside an R-R-taking function is on hr/gravity/resp, which carry one sample per second and so have no ties for stability to preserve. One thing I did and then undid: I generalised the helper behind a Also worth recording, since it is not a defect: 9/9 green. |
Records the HRV ordering fix and credits @ryanbr's issue ryanbr#823 and PR ryanbr#830, which found the magnitude-sort and settled on stamping the order at decode. Verified here against five straps before adopting rather than taken on trust. Also credits @vishk23 for the v18 optical channel corrections (ryanbr#861) and the field inventory (ryanbr#845). States plainly that historical nights keep their stored HRV, and that 4.0 blood oxygen is withheld rather than estimated.
Swift's `sorted(by:)` is documented as NOT stable, so re-sorting R-R intervals by `ts` was free to reorder beats that share a second — reading them in magnitude order instead of emission order. RMSSD is the sum of successive differences, so that reordering changes the number: on the issue's own example 34.85 ms becomes 12.72 ms. Upstream fixed it in 34c66b6 with `sortedByTsStable()`; this fork's 14aa0a0 reverted six call sites, and the 9.3.1 merge resolved SleepStager.swift in favour of the fork side, so it stayed reverted. Restored verbatim from upstream, including the comments explaining why the sort must be stable: SleepStager (detectSleep), SleepStagerV2, AnalyticsEngine (both deep-sleep HRV windows), HRVFreqDomain — via `sortedByTsStable()`; ResonanceEngine via its own spelled-out total comparator, since that file is deliberately free of the WhoopProtocol dependency. Two things this fixes beyond the numbers: the tree was internally inconsistent (HRVAnalyzer and SleepStager's HRV-window path never lost the stable sort, so the same stream was sorted two ways), and Kotlin's `sortedBy` is stable by contract — the Swift side could compute a different HRV than the Android twin for the same night, which the cross-platform parity rule forbids. The five files are now byte-identical to upstream/main again. Verified: swift test in Packages/WhoopProtocol (531 tests) and Packages/StrandAnalytics (1252 tests), both green. No Android twin needed — Kotlin was correct throughout.
9.3.2 DX Beta — build 217, Android versionCode 309. A measurement-correctness release. Three separate faults were feeding wrong numbers into HRV and all three are fixed: the same-second R-R sort this fork reverted from upstream (ryanbr#830), and the Oura beat double-count plus its ordering root cause found upstream (ryanbr#1071, ryanbr#1072). On top of that the fork's own untranslated strings are complete, and ~60 upstream commits land including the sleep-edit wake-date fix (ryanbr#970/ryanbr#1059). What's New generated with Tools/appchangelog-gen.py, so the Swift and Kotlin entries are byte-identical and the Android title ships as a localized string resource rather than a raw Kotlin literal — the i18n gate audits the whole tree, so a hardcoded title there would red-check every later push. Release notes credit third-party contributors by @handle (ryanbr#736): the Oura and HRV work in this release is @pipiche38's, the sleep-edit fix @Creanathan's. Handles collected with Tools/release-contributors.sh v9.3.1-dx-beta. Verified: xcodebuild Strand (macOS) and NOOPiOS (iOS) both BUILD SUCCEEDED at 9.3.2; Tools/i18n_audit.py --ci exits 0 with the six new strings.xml entries in place; swift test green across WhoopProtocol (531), WhoopStore (362), OuraProtocol (168) and StrandAnalytics (1259).
… order that #830 replaced (#976) * docs(data-model): rrInterval still documents the key and read order #830 replaced `docs/DATA_MODEL.md` describes `rrInterval` as it was at v9. Two migrations have changed it since, and the doc states the superseded shape as current: * **Primary key.** Documented `(deviceId, ts, rrMs)`; actual is `(deviceId, ts, rrMs, seq)` since `v24-rr-seq`. `seq` is not in the column table at all. * **Read order.** Documented "Reads order by `ts ASC, rrMs ASC`"; actual is `ts ASC, ord ASC, rrMs ASC, seq ASC` since `v30-rr-ord` (Reads.swift `rrIntervals`, WhoopDao.kt `rrIntervals`). `ord` is not in the column table either. The read-order line is the one worth fixing promptly: `ts ASC, rrMs ASC` is exactly the clause #823 identified as ordering a second's beats by VALUE and biasing RMSSD downward, and #830 replaced it. Anyone reading this doc today is told the biased order is the design. Also records the three properties of `ord` that a consumer cannot infer from the column list, all of them taken from #830 and the source comments rather than restated from scratch: pre-v30 rows are NULL and fall through to the old `(rrMs, seq)` order (so legacy data still reads back with the #823 bias, by design -- it cannot be backfilled); `ord` is batch-local, so a second split across two live flushes restarts at 0 and that second also falls back to magnitude order; and `ord` is a sort key only, present in neither platform's read projection. Two smaller corrections in the same table: * why `seq` keys on `(ts, rrMs)` and not `ts` alone -- the reason it is load-bearing, from WhoopRepository.kt's own warning, so the next person to look at it does not "simplify" it into the data-loss regression. * the migration table is labelled as the selection it already was (it jumps v9 -> v29), and the two RR migrations are added to it under their full identifier strings. That last point is worth stating explicitly because the numeric prefixes are NOT unique -- `v26-cloud-tombstone`/`v26-efficiency-heal`, `v27-apple-step-hour`/`v27-ppg-waveform`, `v28-phone-timezone`/`v28-raw-imu` and `v29-daily-avg-sdnn`/`v29-score-input-provenance` are four live collisions in one `makeMigrator()`. Since GRDB keys a migration by its identifier string, "v30" is ambiguous in a way "v30-rr-ord" is not. Docs only -- no code, no schema, no behaviour change. * docs(data-model): drop the fabricated migration collisions, state the ord projection asymmetry The four cited collisions do not exist upstream. registerMigration is called once per identifier and every numeric prefix is unique, so the list was wrong on its own terms. Keep the guidance it was meant to support — GRDB keys a migration by its identifier string, so cite the identifier in full — and drop the invented evidence. ord is also not symmetric across platforms: Swift selects ts, rrMs and excludes it; WhoopDao.rrIntervals is SELECT * so Room materialises ord into every returned RrInterval. No consumer reads its value on either platform, which is the part that was right.
… order (ryanbr#830) Both platforms read a second's R-R intervals sorted by VALUE (ORDER BY ts, rrMs). Sorting makes adjacent values similar by construction, and RMSSD is built entirely from successive differences, so this biased a headline metric one-directionally down. On the issue's five-beat example: 34.85 ms in emission order, 12.72 ms sorted — a -22.14 ms error, on ~1.1% of same-second groups. Adds `ord`: the beat's position among all beats sharing its ts, stamped at decode time and leading the read sort. Room v23->24, GRDB v30-rr-ord, both additive nullable ALTERs — no table rebuild, no row touched. Not ORDER BY ts, seq. That treats seq as an insertion counter; it is not. assignRrSeq keys on (ts, rrMs), so seq counts repeats of an IDENTICAL beat and every distinct beat in a second carries 0. They would all tie and the order would become undefined — RMSSD could then vary between reads of the same data. ord is not in the key. The key stays (deviceId, ts, rrMs, seq). A per-second insertion counter in the key restarts per batch and collides distinct beats arriving in separate flushes, the data-loss regression assignRrSeq warns about. Legacy rows keep ord NULL: the order was never recorded, so it cannot be backfilled and a guess would assert something nobody observed. SQLite sorts NULL first in ASC, so an all-legacy second ties and falls through to the old (rrMs, seq) order — existing data reads back exactly as before, no COALESCE, no sentinel. Downstream, four unstable sorts would have undone all of it. Every analytics entry point re-sorts by ts defensively, and Swift sorted(by:) is documented as NOT stable — so the emission order could be re-scrambled at the point of use. Stable in practice today (timsort), which is why it would not have surfaced in testing. Kotlin already had the guarantee in writing ("Stable on equal ts", java.util.Arrays.sort); that asymmetry located every site. Adds Array<RRInterval>.sortedByTsStable(), applied at HRVAnalyzer.rollingRmssd, HRVFreqDomain, SleepStager (x2, incl. respRateFromRR whose cumulative-sum beat-time reconstruction is especially order-sensitive), SleepStagerV2, AnalyticsEngine (x2), ShortcutHealthExport. ResonanceEngine carries its own copy: that file is deliberately DB-free and must not gain a module dependency. Also fixes the raw-CSV export, which ordered by ts alone and left same-second beats UNDEFINED — identical data could export two different ways. And corrects the RrInterval doc, which said the Swift key widening was still pending; it shipped as v24-rr-seq. Verified: ORDER BY clauses extracted from both sources, compared token-by-token, then executed against real SQLite — identical, reproducing 34.85 / 12.72 / -22.14. Swift tests run in CI and cover emission order, legacy NULL rows, mixed seconds, repeat-read determinism, ord absent from the key, and stable-sort behaviour. Unverified: the Kotlin half. No SQLite driver on the JVM test classpath, so those tests assert migration shape and the pure stamping function only, and android.yml is disabled so nothing compiles it. The SQL is identical to the clause Swift CI executes. Known residual: a second split across two live flushes restarts ord at 0 and DO NOTHING keeps the first row, so that second falls back to magnitude order. Same batch-local caveat as seq; the historical path delivers a second atomically. Still open from the issue: nobody has measured a full night both ways. 1.1% is a frequency, not an effect size.
… order that ryanbr#830 replaced (ryanbr#976) * docs(data-model): rrInterval still documents the key and read order ryanbr#830 replaced `docs/DATA_MODEL.md` describes `rrInterval` as it was at v9. Two migrations have changed it since, and the doc states the superseded shape as current: * **Primary key.** Documented `(deviceId, ts, rrMs)`; actual is `(deviceId, ts, rrMs, seq)` since `v24-rr-seq`. `seq` is not in the column table at all. * **Read order.** Documented "Reads order by `ts ASC, rrMs ASC`"; actual is `ts ASC, ord ASC, rrMs ASC, seq ASC` since `v30-rr-ord` (Reads.swift `rrIntervals`, WhoopDao.kt `rrIntervals`). `ord` is not in the column table either. The read-order line is the one worth fixing promptly: `ts ASC, rrMs ASC` is exactly the clause ryanbr#823 identified as ordering a second's beats by VALUE and biasing RMSSD downward, and ryanbr#830 replaced it. Anyone reading this doc today is told the biased order is the design. Also records the three properties of `ord` that a consumer cannot infer from the column list, all of them taken from ryanbr#830 and the source comments rather than restated from scratch: pre-v30 rows are NULL and fall through to the old `(rrMs, seq)` order (so legacy data still reads back with the ryanbr#823 bias, by design -- it cannot be backfilled); `ord` is batch-local, so a second split across two live flushes restarts at 0 and that second also falls back to magnitude order; and `ord` is a sort key only, present in neither platform's read projection. Two smaller corrections in the same table: * why `seq` keys on `(ts, rrMs)` and not `ts` alone -- the reason it is load-bearing, from WhoopRepository.kt's own warning, so the next person to look at it does not "simplify" it into the data-loss regression. * the migration table is labelled as the selection it already was (it jumps v9 -> v29), and the two RR migrations are added to it under their full identifier strings. That last point is worth stating explicitly because the numeric prefixes are NOT unique -- `v26-cloud-tombstone`/`v26-efficiency-heal`, `v27-apple-step-hour`/`v27-ppg-waveform`, `v28-phone-timezone`/`v28-raw-imu` and `v29-daily-avg-sdnn`/`v29-score-input-provenance` are four live collisions in one `makeMigrator()`. Since GRDB keys a migration by its identifier string, "v30" is ambiguous in a way "v30-rr-ord" is not. Docs only -- no code, no schema, no behaviour change. * docs(data-model): drop the fabricated migration collisions, state the ord projection asymmetry The four cited collisions do not exist upstream. registerMigration is called once per identifier and every numeric prefix is unique, so the list was wrong on its own terms. Keep the guidance it was meant to support — GRDB keys a migration by its identifier string, so cite the identifier in full — and drop the invented evidence. ord is also not symmetric across platforms: Swift selects ts, rrMs and excludes it; WhoopDao.rrIntervals is SELECT * so Room materialises ord into every returned RrInterval. No consumer reads its value on either platform, which is the part that was right.
Implements #823.
The bug
Both platforms read a second's R-R intervals sorted by value:
Sorting makes adjacent values similar by construction, and RMSSD is built entirely from successive differences. So this is a one-directional downward bias in a headline metric that feeds Charge. On the issue's five-beat example:
~1.1% of same-second groups on 5/MG.
The fix
A new
ordcolumn: the beat's position among all beats sharing itsts, stamped at decode time, leading the read sort.Why not
ORDER BY ts, seq(the fork's two-line version): it assumesseqis an insertion counter. It isn't —assignRrSeqkeys on(ts, rrMs), soseqcounts repeats of an identical beat and every distinct beat in a second carries0. They'd all tie and the order would become whatever the scan returns, trading a deterministic wrong order for an undefined one. RMSSD could then vary between reads of the same data.Why
ordisn't in the key. The key stays(deviceId, ts, rrMs, seq). A per-second insertion counter in the key restarts per batch and collides distinct beats arriving in separate flushes — the data-loss regressionassignRrSeq's own comment warns about.ordnever decides which rows survive.Legacy rows stay NULL. The order was never recorded, so it can't be backfilled, and a guess would assert something nobody observed. SQLite sorts NULL first in ASC, so an all-legacy second ties on
ordand falls through to the old(rrMs, seq)order — existing data reads back exactly as before. NoCOALESCE, no sentinel.Parity
Room v23→24 and GRDB
v30-rr-ord, both additive nullableALTERs — no table rebuild, no row touched. I extracted theORDER BYfrom both sources and compared token-by-token, then ran both against real SQLite:Also in scope
tsalone, leaving same-second beats undefined — identical data could export two different ways. Now uses the same clause.RrInterval's PARITY NOTE said the Swift key widening was still pending. It shipped asv24-rr-seq.Testing
Swift tests run in CI (
swift-packages.ymlcoversPackages/**) and cover this end-to-end: emission order restored, RMSSD back to 34.85, legacy NULL rows unchanged, mixed seconds deterministic across repeated reads,ordabsent from the primary key. Added one test-only seam to write a legacy NULL-ordrow, since the normal path always stamps it and the existing-data fallback is the branch most worth testing rather than assuming.Checked every existing R-R read assertion:
ReadTestsseeds ascending so it still passes (now for a different reason — noted in a comment), and the store-backed analytics test seeds one beat per second, soordis always 0.Not verified: the Kotlin half is unrun. There's no SQLite driver on the JVM unit-test classpath, so those tests assert migration shape and the pure stamping function only, and
android.ymlis disabled so nothing compiles it. The SQL itself is identical to the Swift clause that CI does execute.Performance: the sort no longer matches the PK index
(ts, rrMs, seq), so SQLite sorts. At this query's size (a night, ceiling 200k rows) that's tens of ms; I'd rather measure than add a covering index to a large table pre-emptively.Open question from the issue that this does not settle: whether a real night shows the 1.1% figure translating into a meaningful nightly RMSSD delta. The fix is correct regardless, but that number would tell us whether it matters clinically.