fix(import): store Oura efficiency as a 0-1 fraction, matching the native column - #376
Conversation
…tive column
The Oura API importer wrote Oura's native 0-100 integer `efficiency`
straight into sleepSession.efficiency / dailyMetric.efficiency, but
NOOP's own sleep pipeline (StrandAnalytics) stores that same shared
column as a 0-1 FRACTION everywhere it computes it (asleep / in-bed —
AnalyticsEngine.swift, SleepStageTotals.swift). Real WHOOP-era rows sit
at 0.36-0.94; real imported Oura rows sat at 83-96 - same column, two
scales. The UI already carries defensive normalization shims for this
exact inconsistency (SleepView.swift's `if e > 1.5 { e /= 100 }`,
WatchSessionBridge.swift's matching comment), which is how the mismatch
was traced back to the importer.
Normalize at the parse boundary in OuraApiParser.swift: divide by 100
where `efficiency` is read off the session. The day rollup derives its
efficiencyPct from the same session value, so both the per-session and
daily rows come out as fractions with a single change.
Add a data-heal migration (v26) for rows already written with the old
scale: an UPDATE-only pass, no schema change, dividing efficiency by
100 wherever deviceId = 'oura-api' AND efficiency > 1.5 - a threshold
no genuine fraction exceeds and no genuine Oura percent falls under, so
it's idempotent and can't touch an already-correct row. Scoped to
deviceId so WHOOP-native and other-brand rows are never touched. No
Android migration twin: android/'s `com.noop.oura` package is the BLE
ring driver, not a REST/API cloud importer - there is nothing to heal
there.
The WHOOP CSV importer had the same unit bug as the Oura API importer: "Sleep efficiency %" (0-100) written straight into the 0-1 fraction columns, on both platforms. Fix at the same write boundary as the Day Strain rescale, with the inverse on the CSV exporters so a NOOP->NOOP round-trip stays lossless (percent rounded to 4 decimals at emission -- num() prints shortest-round-trip Doubles, and raw fraction*100 leaks FP dust like 92.30000000000001 into the cell). The heal migration drops its deviceId scoping (now v26-efficiency-heal): both known percent writers are corrected by the same >1.5 predicate, so CSV-imported rows under arbitrary strap deviceIds heal alongside oura-api rows. Android gets the identical importer/exporter boundary fix (byte-parity helpers); healing Android's historical Room rows is left to a maintainer-owned Room migration, called out in the PR.
|
Pushed f1b0623 expanding this to the full efficiency-unit story after an audit of real data turned up the sibling: the WHOOP CSV importer wrote "Sleep efficiency %" (0–100) into the same 0–1 fraction columns — identical bug class, both platforms. What the commit adds:
Android caveats, honestly: the Kotlin changes are careful transcription (I have no Android toolchain here) — |
|
Excellent diagnosis and a comprehensive core fix — the efficiency scale is normalized at every boundary (Oura API import, WHOOP CSV import + export) on both platforms, byte-parity, and the native-convention evidence (AnalyticsEngine:425 / SleepStageTotals:112 + the existing One thing I'd want closed before merge, though. The Android Room heal isn't optional — and it compoundsYou flag it in the migration comment ("No Android Room migration twin in this PR…"), but I don't think it can be deferred, for two reasons:
The write-boundary fix is correctly twinned on both platforms; the heal needs to be too. Suggest adding the Room heal twin in this PR — a Smaller notes
Core fix is right and well-verified — just want the stored-data heal to land on both platforms so iOS and Android don't diverge (and so the Android export doesn't double-scale historical rows). |
…y rows The Room heal twin was flagged in review as required, not optional: unhealed Android percent-scale efficiency rows (92) would export as 9200 via the new x100 CSV exporter, and iOS/Android stored data would diverge for the same imported CSV. Adds MIGRATION_18_19 (bumps @database version 18 -> 19), the byte-parity twin of WhoopStore's GRDB v26-efficiency-heal: UPDATE-only, no schema change, dividing sleepSession.efficiency and dailyMetric.efficiency by 100 wherever > 1.5, not deviceId-scoped, idempotent. Pinned by a new plain-JVM test (EfficiencyHealMigrationTest) following this codebase's existing Room-migration-test convention (string-pinned SQL + version-pair assertions; no Robolectric/JDBC-SQLite harness here to execute against a real database). Sequencing caveat carried into the PR description: this claims Room's next free slot (18->19) as of now, and collides with any other pending PR wanting the same slot, same as the Swift v26 GRDB slot's collision risk against other pending PRs -- whichever lands second needs to renumber. No Android toolchain available to run testFullDebugUnitTest here; transcribed carefully against the existing Migration object style, called out in the PR comment.
|
Pushed 86ae44a adding the Room heal migration, per review — agreed it's required, not optional. What changed:
Sequencing caveat (per your flag): this claims Room's next free slot (18→19) as of now. Same collision risk you called out for the GRDB What I could not verify: no Android toolchain in this environment — no JDK ( Swift suites untouched by this commit; re-ran both anyway to confirm the branch tip is still green: StrandImport 200/200 (1 skipped by design), WhoopStore 257/257. |
tigercraft4
left a comment
There was a problem hiding this comment.
Review (external, non-blocking). Fix logic (Oura efficiency as 0-1 fraction) looks right; scope growing to the WHOOP CSV lane is reasonable (same shared column). Two inline notes below.
| } | ||
|
|
||
| /** | ||
| * v18 -> v19: Oura/WHOOP efficiency-unit HEAL, the Room twin of the Swift WhoopStore v26 |
There was a problem hiding this comment.
This Room migration is real and present — but per your own PR note it was "reviewed, not compiled or executed" (no Android toolchain available). Please confirm ./gradlew testFullDebugUnitTest (specifically EfficiencyHealMigrationTest) actually runs green before merge. Also flagging your own note about a possible Room migration-version-slot collision with other pending PRs — needs coordination with the maintainer on merge order.
| // already-correct row and a second run finds nothing left: idempotent. Deliberately NOT | ||
| // deviceId-scoped: both known percent writers are healed by the same predicate — the Oura API | ||
| // importer ('oura-api' rows) and the WHOOP CSV importer (rows under whatever strap deviceId the | ||
| // user imported into). No Android Room migration twin in this PR: the Kotlin CSV importer gets |
There was a problem hiding this comment.
This comment says "No Android Room migration twin in this PR" — but this same PR adds exactly that twin (WhoopDatabase.kt EFFICIENCY_HEAL_MIGRATION_SQL, v18->v19). Looks like a leftover from an earlier draft; worth removing/updating so it doesn t mislead the next reader.
…tive column (ryanbr#376) * fix(import): store Oura efficiency as a 0-1 fraction, matching the native column The Oura API importer wrote Oura's native 0-100 integer `efficiency` straight into sleepSession.efficiency / dailyMetric.efficiency, but NOOP's own sleep pipeline (StrandAnalytics) stores that same shared column as a 0-1 FRACTION everywhere it computes it (asleep / in-bed — AnalyticsEngine.swift, SleepStageTotals.swift). Real WHOOP-era rows sit at 0.36-0.94; real imported Oura rows sat at 83-96 - same column, two scales. The UI already carries defensive normalization shims for this exact inconsistency (SleepView.swift's `if e > 1.5 { e /= 100 }`, WatchSessionBridge.swift's matching comment), which is how the mismatch was traced back to the importer. Normalize at the parse boundary in OuraApiParser.swift: divide by 100 where `efficiency` is read off the session. The day rollup derives its efficiencyPct from the same session value, so both the per-session and daily rows come out as fractions with a single change. Add a data-heal migration (v26) for rows already written with the old scale: an UPDATE-only pass, no schema change, dividing efficiency by 100 wherever deviceId = 'oura-api' AND efficiency > 1.5 - a threshold no genuine fraction exceeds and no genuine Oura percent falls under, so it's idempotent and can't touch an already-correct row. Scoped to deviceId so WHOOP-native and other-brand rows are never touched. No Android migration twin: android/'s `com.noop.oura` package is the BLE ring driver, not a REST/API cloud importer - there is nothing to heal there. * fix(import): normalize WHOOP CSV efficiency to the native fraction too The WHOOP CSV importer had the same unit bug as the Oura API importer: "Sleep efficiency %" (0-100) written straight into the 0-1 fraction columns, on both platforms. Fix at the same write boundary as the Day Strain rescale, with the inverse on the CSV exporters so a NOOP->NOOP round-trip stays lossless (percent rounded to 4 decimals at emission -- num() prints shortest-round-trip Doubles, and raw fraction*100 leaks FP dust like 92.30000000000001 into the cell). The heal migration drops its deviceId scoping (now v26-efficiency-heal): both known percent writers are corrected by the same >1.5 predicate, so CSV-imported rows under arbitrary strap deviceIds heal alongside oura-api rows. Android gets the identical importer/exporter boundary fix (byte-parity helpers); healing Android's historical Room rows is left to a maintainer-owned Room migration, called out in the PR. * fix(android): add the Room heal migration for percent-scale efficiency rows The Room heal twin was flagged in review as required, not optional: unhealed Android percent-scale efficiency rows (92) would export as 9200 via the new x100 CSV exporter, and iOS/Android stored data would diverge for the same imported CSV. Adds MIGRATION_18_19 (bumps @database version 18 -> 19), the byte-parity twin of WhoopStore's GRDB v26-efficiency-heal: UPDATE-only, no schema change, dividing sleepSession.efficiency and dailyMetric.efficiency by 100 wherever > 1.5, not deviceId-scoped, idempotent. Pinned by a new plain-JVM test (EfficiencyHealMigrationTest) following this codebase's existing Room-migration-test convention (string-pinned SQL + version-pair assertions; no Robolectric/JDBC-SQLite harness here to execute against a real database). Sequencing caveat carried into the PR description: this claims Room's next free slot (18->19) as of now, and collides with any other pending PR wanting the same slot, same as the Swift v26 GRDB slot's collision risk against other pending PRs -- whichever lands second needs to renumber. No Android toolchain available to run testFullDebugUnitTest here; transcribed carefully against the existing Migration object style, called out in the PR comment.
What
Scope (updated — this PR grew beyond the original title): normalizes the
efficiencycolumn to asingle 0-1 fraction convention across both import lanes and both platforms:
efficiencyfield — a 0-100 integer percent — straightinto
sleepSession.efficiency/dailyMetric.efficiency(OuraApiParser.swift).the same 0-1 fraction columns, on both Swift and Kotlin.
NOOP's own sleep pipeline stores that shared column as a 0-1 fraction everywhere it computes it, so
rows from either importer ended up in the same column at two different scales, on both platforms.
Native convention (verified, not assumed)
Packages/StrandAnalytics/Sources/StrandAnalytics/AnalyticsEngine.swift:425computes
let efficiency = inBedS > 0 ? effWeighted / inBedS : 0.0— aratio of two second-counts, always in
[0,1]— and writes it directlyinto both
DailyMetric.efficiency(AnalyticsEngine.swift:698) andCachedSleepSession.efficiency(AnalyticsEngine.swift:721).Packages/StrandAnalytics/Sources/StrandAnalytics/SleepStageTotals.swift:112computes the same ratio (
total.asleep / total.inBed) independently. RealWHOOP-era rows in an actual on-device database sit at 0.36-0.94; real
imported Oura rows sat at 83-96 — same column, two scales.
This inconsistency already left fingerprints in the UI, which is how it
was traced back to the importer rather than assumed:
Strand/Screens/SleepView.swift:1173—if e > 1.5 { e /= 100 } // efficiency arrives as % on some import pathsStrand/Screens/SleepView.swift:2378-2380—efficiencyPct(_:)defensively does
stored <= 1.0 ? stored * 100 : storedStrand/Data/WatchSessionBridge.swift:198-202— explicit comment:"efficiency is stored as a fraction in [0,1] in some paths and as a
percent in others"
Both shims treat
<= 1.0as the correct/expected case, confirming thefraction is the native convention, not the percent.
Fix
Oura API lane: normalize at the parse boundary in
OuraApiParser.swift: divide by 100 whereefficiencyis read off the session (WearableJSON.posDbl(s, "efficiency").map { $0 / 100.0 }). The dayrollup's
efficiencyPctis derived from that same session value, so both the per-session row and thedaily rollup come out as fractions from one change.
WHOOP CSV lane (both platforms): the importer's "Sleep efficiency %" (0-100) is now divided at the
WRITE boundary —
WhoopExportImporter.fractionFromImportedEfficiencyPct(Swift, used byStrand/Data/WhoopImporter.swift) andWhoopCsvImporter.efficiencyFractionFromPct(Kotlin) — the sameshape as the existing Day Strain ⇄ Effort rescale, so the verbatim parsed percent value is preserved and
only the store boundary changes. The CSV exporters apply the inverse
(
WhoopExportImporter.whoopEfficiencyPctFromFraction/ the Kotlin twin inWhoopCsvExporter.kt) so anexported CSV stays genuinely WHOOP-format and a NOOP→NOOP round-trip is lossless — rounded to 4 decimals
of a percent at emission, because
num()prints shortest-round-trip Doubles and a rawfraction * 100leaks FP dust (e.g.
92.30000000000001) into the cell otherwise.OuraExportParser.swift(the separate Oura file-based account-export lane) reads the same rawefficiencyfield independently and is not touched by this PR — it's a third lane with its ownpercent/fraction question, left for a follow-up.
Data heal (migration, not just re-import) — now on BOTH platforms
Re-running an import wouldn't fix already-stored bad rows on its own:
dailyMetric/sleepSessionareupserted with the importer's value on conflict, so simply re-syncing would overwrite a bad row with
another bad row until the parser fix above landed — a schema-level heal is required.
iOS/macOS (GRDB):
Packages/WhoopStore/Sources/WhoopStore/Database.swift, migrationv26-efficiency-heal(appended afterv25-oura-raw):value falls at or below 1.5, so the predicate can never touch an already-correct row, and a second run
is a no-op.
deviceId = 'oura-api'version): both knownpercent writers — the Oura API importer's
oura-apirows AND the WHOOP CSV importer's rows underwhatever strap deviceId the user imported into — are healed by the same predicate.
Android (Room) — added in this PR, was previously missing: without this, an Android user's stored
efficiency would permanently diverge from an iOS user's for the same imported CSV (iOS heals via v26,
Android wouldn't), and worse, the new Android CSV exporter's
efficiency * 100would turn an unhealedpercent row (
92) into9200in an exported CSV.android/app/src/main/java/com/noop/data/WhoopDatabase.kt,migration
MIGRATION_18_19(bumps@Database(version = 18)→19), the byte-parity twin of the GRDBheal — same tables, same threshold, same non-scoped predicate:
UPDATE-only, no schema/column change, so no schema-export/column-order concern (Room's
exportSchema = false, noandroid/schemas/in this repo). Pinned by a new plain-JVM test,android/app/src/test/java/com/noop/data/EfficiencyHealMigrationTest.kt, following this codebase'sexisting Room-migration-test convention (string-pinned SQL + version-pair assertions — there's no
Robolectric/JDBC-SQLite harness here to execute the migration against a real database).
Sequencing caveat (flagged in review, not this PR's fault):
v26(GRDB) collides with two otherpending PRs that also want the next GRDB slot, and Room's
18→19slot likewise collides with anotherpending Room migration. Whichever of the colliding PRs lands second will need to renumber — worth
sequencing deliberately rather than merging blind.
Android parity
The WHOOP CSV importer/exporter write-boundary fix and the Room heal above are now BOTH on Android in this
PR (see
android/app/src/main/java/com/noop/ingest/WhoopCsvImporter.kt,android/app/src/main/java/com/noop/ingest/WhoopCsvExporter.kt, andandroid/app/src/main/java/com/noop/data/WhoopDatabase.kt).The Oura API importer specifically has no Android equivalent to fix:
android/'scom.noop.ourapackage (
OuraDriver.kt,OuraGatt.kt,Framing.kt,Decoders.kt,Auth.kt,RingGen.kt) is the BLEring protocol driver (headless, no
android.bluetoothin the package itself) — a parallel, unrelatedfeature for pairing directly with an Oura ring over Bluetooth, not a cloud API import path. No
ouraring.com/OAuth/cloud-import references exist inandroid/outside that BLE driver, so there is noOura-API-shaped data on Android for the Room heal to worry about — only the WHOOP-CSV-shaped percent rows,
which the heal above covers.
Caveat — I have no Android toolchain in this environment (no JDK;
./gradlewcan't even report--version). The Kotlin write-boundary fix (efficiencyFractionFromPct/ the exporter'sround(it * 100.0 * 10_000) / 10_000) and theMIGRATION_18_19heal + its test are careful transcriptions of theSwift logic and this codebase's existing Room-migration style, reviewed closely but not compiled or
executed.
testFullDebugUnitTestneeds to run on a real toolchain before merge.Tests
Packages/StrandImport/Tests/StrandImportTests/OuraApiParserSleepTests.swift:testEfficiencyIsNormalizedToZeroToOneFraction— a raw"efficiency": 92input yields
0.92on both the session and the day rollup.Packages/StrandImport/Tests/StrandImportTests/WhoopCsvExporterTests.swift:testEfficiencyPctFractionPairIsLosslessAndNilSafe— the fraction↔percent round-trip is lossless(including the FP-dust rounding), and the existing round-trip tests' fixtures now seed
efficiencyasthe native 0-1 fraction.
Packages/WhoopStore/Tests/WhoopStoreTests/MigrationTests.swift:testV26HealsEfficiencyPercentToFraction— seeds rows at the pre-v26 (v25-oura-raw) schema state viaWhoopStore.makeMigrator().migrate(_:upTo:), then applies the rest of the migrator and confirms: anoura-apipercent row heals, an already-fractionoura-apirow is untouched (idempotent), aWHOOP-CSV-imported percent row under an arbitrary strap deviceId (
my-whoop) ALSO heals (no longerdeviceId-scoped), and a native fraction row under a strap deviceId is left alone.
android/app/src/test/java/com/noop/ingest/WhoopCsvExporterTest.kt— its existing round-trip fixtures(
cyclesRoundTripThroughRealParseretc.) now seedefficiencyas the native 0-1 fraction too.android/app/src/test/java/com/noop/data/EfficiencyHealMigrationTest.kt— pinsMIGRATION_18_19's version pair, exact SQL for both tables, that it's UPDATE-only (no schemamutation), and that the threshold/divisor/lack-of-deviceId-scoping matches the Swift heal.
Verification:
No regressions in either package (re-verified on the current branch tip). Android: no toolchain available
here —
testFullDebugUnitTest(including the newEfficiencyHealMigrationTest) needs to run on a realbuild machine before merge; flagging honestly rather than claiming untested Kotlin as tested.
Honest verification
above (StrandAnalytics writer + two independent UI shims), not assumed
from the bug description.
TodayView.swift:4234'srestCaptionfallback formatsd.efficiencywith%.0f%% effwith no normalization at all (unlikethe two shims cited above) — a narrower, rare-path inconsistency (it
only fires when
totalSleepMinis nil butefficiencyisn't) leftalone here as out of scope.
caveat under "Android parity" above.