fix(workouts): count PPG-derived HR in the workout window stats#841
Conversation
The chart and the average read different tables. The chart uses WhoopDao.hrSamples, which UNIONs measured hrSample with PPG-derived ppgHrSample (#156). hrWindowStats, which backs the Avg HR, read hrSample alone. fillWorkoutHrFromStrap needs 60 samples before it shows a number. On a WHOOP 5 the firmware banks v26 PPG instead of v18 HR per second, so a PPG-heavy workout draws a full chart and still counts under 60 measured rows — Avg HR renders blank. That is exactly the reporter's "as the HR data is shown on the chart it can be calculated", and it explains why only SOME workouts are affected: it depends on how PPG-heavy that particular window is. Manual and Health Connect rows both hit it because both pass through the same gate. iOS never had this. Its twin reduces store.hrSamples, the coalescing read — so this was also a parity break: same data, different number per platform. The Android query looks like a deliberate optimisation, aggregate in SQL rather than materialise up to 8000 rows, and its own comment sells that ("no hrSamples LIMIT truncation"). It is the better shape; it just dropped the PPG union on the way. Now aggregates the same measured-∪-PPG row set, with the identical anti-join, so a measured second is never double-counted by its PPG estimate. maxHr can now be a PPG-derived peak — correct, and the point: the whole function exists to keep the shown number agreeing with the graph and zones, which already include those rows. Verified against real SQLite with the SQL extracted from source: on a 20-measured / 400-PPG window the aggregate returns n=415 avg=141.98 max=146, identical to reducing the chart's union, where the old query returned n=20 and fell below the floor. Not verified by CI: no SQLite driver on the JVM unit-test classpath and android.yml is disabled. Single-query change, only caller is fillWorkoutHrFromStrap.
d19cf74 to
90edfa2
Compare
|
Re-review. No defect found; two things worth adding to the record, one of which widens what this repairs. 1. It is not only Avg HR — Effort is blocked by the same gate. 2. The function was already internally inconsistent, which is corroboration. Ten lines below the old measured-only Query plan, since replacing a single indexed aggregate with a union + anti-join invites the obvious objection: Both branches are indexed SEARCHes on their PKs, and the anti-join is a covering-index point lookup per PPG row — not a scan. Safe on the first-paint path this runs on. One user-visible consequence to expect: strap-native workouts always recompute Avg HR from the trace, so any whose window had PPG seconds will now show a different number than before. That is the correction — the displayed value starts agreeing with the graph — but numbers people have already seen will move. Imported rows are unaffected: they are only filled when null, never overridden. |
…836) The Android half of this PR made the Kotlin aggregate count PPG-derived seconds. That left the twins disagreeing a second way: Swift reduced store.hrSamples(limit: 8000), so any workout longer than ~2h13m at 1 Hz reported the mean of its FIRST 8000 samples as the session average. On a 3h session with drifting HR that is 131 bpm against a true 135 — wrong on its own terms, not merely divergent. Adds WhoopStore.hrWindowStats, the twin of WhoopDao.hrWindowStats: same measured-∪-PPG union, same anti-join, aggregated in SQLite over the whole window with no row limit. Swift keeps CAST(ROUND(p.bpm) AS INTEGER) because its ppgHrSample.bpm column is REAL where Kotlin's is INTEGER; both decoders emit whole bpm (#219), so the values agree and the two queries return identical results on their own column types — verified by running each against its platform's schema: (415, 141.8867469879518, 146) both. Deliberately put in WhoopStore rather than Repository: swift-packages.yml runs Packages/** tests, while Strand/ is the app target that nothing compiles. So the query and its five tests get real CI coverage, which the old in-Repository reduce never had. Also faster on the path this function exists to protect. #833 moved an 8000-row materialise-and-reduce off the main actor to stop a first-paint freeze; now the common case (no strain fill) materialises no rows at all. Rows are read only when a strain backfill needs the series, where the 8000 cap correctly stays — it bounds the strain window, not the average. That mirrors Kotlin exactly. Retires Repository.reduceWorkoutHr, which had no callers left, and its test file with it. What it guarded (mean rounding) is now covered by the WhoopStore tests, which CI actually executes — StrandTests does not run anywhere today.
|
Folded the follow-up in — this is now a both-platforms fix rather than half of one. Swift had its own bug, not just a divergence. It reduced Adds Put in It also makes #833 better, not worse. That freeze fix moved an 8000-row materialise-and-reduce off the main actor; now the common path (no strain fill) materialises no rows at all. Rows are read only when a strain backfill needs the series — where the 8000 cap correctly stays, since it bounds the strain window, not the average. Same shape as Kotlin. On byte-parity: the two SQL strings are deliberately not identical. Swift keeps Removed: 9/9 green. |
… HRR (#857) Four surfaces on one workout card, no two agreeing. Swift avg workoutHrDeviceId (single) under-reads imported after a strap re-add Swift chart day union WRONG STRAP for a bout detected on a 2nd WHOOP Swift zones day union same Swift HRR day union same Kotlin chart single active id both failure modes Kotlin zones single active id both Kotlin HRR day union wrong strap for detected So on iOS the Avg HR and the HR curve on the same card could describe DIFFERENT straps. Not a Kotlin-vs-Swift divergence — a live self-inconsistency. Neither existing rule is right on its own. The correct read is source-class-dependent, which workoutHrDeviceId already encoded; it just returned one id where one case needs two: detected -> [recording strap] a union mixes in a strap that never recorded it (#510) manual / imported -> active u canonical no strap of its own, and the worn strap may bank under either after a re-add (#814) workoutHrDeviceIds returns that list on both platforms and all FOUR surfaces consume it: chart, zone minutes, Avg HR / Effort, and heart-rate recovery. HRR reads past the bout, which is the one reason it might have wanted different treatment; it does not, since the strap that recorded the session is still on the wrist minutes later. The aggregate needed real work. A naive `deviceId IN (...)` double-counts a second banked under both ids — inflating n and skewing avg, silently, because both numbers stay plausible. GROUP BY ts with MIN(pri) keeps one row per second and takes the primary is, matching the dedup the chart already did. Fixed 2-arity rather than a bound collection: importedReadIds is 1 or 2 by construction, and Room @query is static SQL that cannot express per-id priority through an IN list. Passing the same id twice is byte-identical to the old single-id read, so a single-WHOOP install is unchanged and needs no special case. That is the control test. The aggregate lives in WhoopStore, where swift-packages.yml actually runs its tests — the placement that made #841 Swift half verifiable and its Kotlin half not. Verified against SQLite before commit, and the Kotlin @query was extracted from source and executed to confirm it returns the same numbers as the Swift twin: A(0..9) + B(5..19), A primary -> n=20 avg=150.0 max=200 (naive IN would say 25) B primary -> n=20 avg=175.0 PPG anti-join per id -> n=15 max=130 same id twice -> identical to the pre-change read Tests: four added in WhoopStoreTests (overlap, precedence, per-id PPG anti-join, single-id control); five existing moved to the two-id signature with assertions untouched; both resolver tests moved to the list API, plus two new Swift cases for the behaviour the rename exists for — an imported row getting the full union on a multi-namespace install, and a detected row still resolving to one id on that same install. Day-level hrSamplesUnion / hrBucketsUnion are untouched and still used by Today, the full-day chart and the auto-workout nudge, where the union is the correct read. Swift WorkoutRow carries no deviceId, so its resolver takes only source where Kotlin also takes rowDeviceId — pre-existing, documented in the original #510 note. NOT field-verified: the bug only manifests on a multi-strap install, which nobody here has. The two-id path is covered by CI tests against real SQLite and a single-strap control proving the common case is byte-identical, but no one has confirmed it on two straps. The Kotlin half is not compiled or run by CI.
1 participant
Fixes #836.
The bug
The chart and the average read different tables.
WhoopDao.hrSampleshrSamplewithppgHrSample(#156)WhoopDao.hrWindowStatsFROM hrSamplealonefillWorkoutHrFromStrapneeds 60 samples before it will show a number. On a WHOOP 5 the firmware banks v26 PPG instead of v18 HR per second, so a PPG-heavy workout draws a full chart and still counts under 60 measured rows — Avg HR renders blank.That is precisely the reporter's "as the HR data is shown on the chart it can be calculated", and it explains why only some workouts are affected: it depends how PPG-heavy that particular window is. Manual and Health Connect rows both hit it because both pass through the same gate.
iOS never had this. Its twin reduces
store.hrSamples, the coalescing read. So this was also a parity break — same data, different number per platform.The fix
The Android query looks like a deliberate optimisation — aggregate in SQL rather than materialise up to 8000 rows — and its own comment sells that ("no
hrSamplesLIMIT truncation"). That's the better shape; it just dropped the PPG union on the way. Now it aggregates the same measured-∪-PPG row set, with the identical anti-join, so a measured second is never double-counted by its PPG estimate.maxHrcan now be a PPG-derived peak. That's correct and rather the point — this function exists to keep the shown number agreeing with the graph and zones, which already include those rows.Verification
SQL extracted from source and run against real SQLite, on a 20-measured / 400-PPG window with 5 overlapping seconds:
Not verified by CI: no SQLite driver on the JVM unit-test classpath and
android.ymlis disabled, so nothing executes or compiles this. Single-query change;fillWorkoutHrFromStrapis the only caller. No test pinned the old SQL.Known residual, not fixed here
Now that Kotlin aggregates the whole window while Swift reduces
store.hrSamples(limit: 8000), the two diverge for workouts longer than ~2h13m at 1 Hz: Swift averages the first 8000 samples, Kotlin the lot. That's pre-existing — this PR makes it more visible rather than causing it. Fixing it properly means giving Swift the same aggregate query, which is a real change to a carefully off-main-actor'd path and doesn't belong in a bug fix. Happy to file it.Also unconfirmed: whether the reporter's specific workouts are PPG-heavy. I've asked for the export on the issue. The code path is unambiguous, so I'm not blocking on it.