From a8a13d6cc93305663b98afd65bc094c0571cc53e Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Fri, 28 Aug 2026 21:21:04 +1200 Subject: [PATCH 1/2] analytics: reduce a skin-temp window to one scale before anything aggregates it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported by @pipiche38 in #1705, with the file and line references already done. skin_temp is deliberately bimodal: a WHOOP CSV import writes ABSOLUTE wrist temperature into the same field the computed pipeline fills with a signed deviation from the personal baseline. Formatting survives that, because the per-value formatter re-derives the kind for each number, which is exactly why nothing looks broken. Aggregates do not. A min, max, mean or window-over-window delta drawn across both kinds is arithmetic on two different scales, and the result is plausible rather than obviously wrong: on the reported account a handful of ~34 C readings lift a should-be-near-zero deviation average past a full degree, and the mean is then itself below 20, so it gets labelled with the deviation unit. The guard for this already existed and both Explorer paths had simply never adopted it. VitalSignsSummary and HealthVitalsLogic pick one kind and filter their series to it before touching a sparkline; SkinTempDisplay already models the kind, on both platforms, with tests. So this adds one decision beside it — dominantKind, the kind a mixed window reduces to, which is the newest entry's, matching what those two already do — rather than a fourth inline copy of the >= 20 check. Applied at the ONE place each platform merges the layers, not at each screen: Swift in Repository.exploreSeries where the three layers collapse, Kotlin in WhoopRepository.resolvedSeries where the imported and computed identities are resolved first-wins. That covers every consumer at once, including two the report did not have to name: the Explorer correlation scan, and Compare's min-max chart normalisation on Android. Reducing rather than converting is deliberate. Converting the import to a deviation at write time would fix every downstream reader too, but it rewrites stored data irreversibly, needs a baseline a fresh import-only install does not have, and puts both platforms' baseline derivation under the byte-identical stored-data contract. Filtering keeps the newest reading intact and drops only older entries of the other kind. The regression guard is the one the report asked for, built from its own numbers — the 313-row 32.39..36.64 import against computed deviations inside +/-0.3. It asserts the filtered window is single-scale and pins the defect itself: unfiltered, that mean lands at 15.43 and is labelled as a deviation because it falls below 20. Verified: 4,725 Android tests, 0 failures with --rerun-tasks --no-build-cache; the Swift decision compiled standalone and agreed with the Kotlin. Both twins live in package/analytics code, so default CI covers them without app-build. --- .../StrandAnalytics/SkinTempDisplay.swift | 20 +++++++++++ .../SkinTempDisplayTests.swift | 33 +++++++++++++++++ Strand/Data/Repository.swift | 11 +++++- .../com/noop/analytics/SkinTempDisplay.kt | 21 +++++++++++ .../java/com/noop/data/WhoopRepository.kt | 19 +++++++++- .../com/noop/analytics/SkinTempDisplayTest.kt | 35 ++++++++++++++++++- 6 files changed, 136 insertions(+), 3 deletions(-) diff --git a/Packages/StrandAnalytics/Sources/StrandAnalytics/SkinTempDisplay.swift b/Packages/StrandAnalytics/Sources/StrandAnalytics/SkinTempDisplay.swift index 3f71229e92..5b8f2aa018 100644 --- a/Packages/StrandAnalytics/Sources/StrandAnalytics/SkinTempDisplay.swift +++ b/Packages/StrandAnalytics/Sources/StrandAnalytics/SkinTempDisplay.swift @@ -22,6 +22,26 @@ public enum SkinTempDisplay { VitalBands.isAbsoluteSkinTemp(value) ? .absolute : .deviation } + /// The one kind a MIXED series must be reduced to before any aggregate is taken (#1705). + /// + /// A CSV import writes absolute °C and the computed pipeline writes a deviation, both into this + /// same field, so one window can hold both. Formatting stays honest per value — `kind(of:)` is + /// re-derived for each — but a min, max, mean or window-over-window delta drawn across both is + /// arithmetic on two different scales, and it looks plausible: a handful of ~34 °C readings lift a + /// should-be-near-zero deviation average past a full degree, and the mean is then itself below 20 + /// so it gets labelled Δ°C. + /// + /// The newest entry decides, matching what `VitalSignsSummary` already does for its sparkline — + /// the reading the user is actually looking at sets the scale, and older entries of the other kind + /// drop out rather than being converted, because converting needs a baseline that a fresh + /// import-only install does not have. + /// + /// - Parameter values: the window's values, ascending by day. + /// - Returns: the kind to keep, or nil for an empty window. + public static func dominantKind(valuesAscendingByDay values: [Double]) -> Kind? { + values.last.map(kind(of:)) + } + /// Trailing unit chip: `"°C"` / `"°F"` for absolute, `"Δ°C"` / `"Δ°F"` for deviation. public static func unitSymbol(kind: Kind, fahrenheit: Bool) -> String { let base = fahrenheit ? "°F" : "°C" diff --git a/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SkinTempDisplayTests.swift b/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SkinTempDisplayTests.swift index a42d607409..2793cd2859 100644 --- a/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SkinTempDisplayTests.swift +++ b/Packages/StrandAnalytics/Tests/StrandAnalyticsTests/SkinTempDisplayTests.swift @@ -66,4 +66,37 @@ final class SkinTempDisplayTests: XCTestCase { XCTAssertEqual(SkinTempDisplay.kind(of: v) == .absolute, abs, "v=\(v)") } } + + // MARK: - dominantKind (#1705) + + func testDominantKindIsTheNewestEntrys() { + XCTAssertEqual(SkinTempDisplay.dominantKind(valuesAscendingByDay: [34.6, 35.1, -0.2]), .deviation) + XCTAssertEqual(SkinTempDisplay.dominantKind(valuesAscendingByDay: [-0.2, 0.1, 34.6]), .absolute) + XCTAssertNil(SkinTempDisplay.dominantKind(valuesAscendingByDay: [])) + } + + func testDominantKindOfASingleKindWindowIsThatKind() { + XCTAssertEqual(SkinTempDisplay.dominantKind(valuesAscendingByDay: [-0.24, 0.21, 0.01]), .deviation) + XCTAssertEqual(SkinTempDisplay.dominantKind(valuesAscendingByDay: [32.39, 34.60, 36.64]), .absolute) + } + + /// The regression guard the issue asked for: filtering by `dominantKind` must leave a window that + /// no aggregate can straddle. Values are the reported ones — a 313-row absolute import + /// (32.39…36.64, mean 34.60) coexisting with computed deviations inside ±0.3. + func testFilteringByDominantKindLeavesOneScale() { + let mixed: [Double] = [34.60, 35.12, 32.39, 36.64, -0.24, 0.21, 0.01, 0.11, 0.0] + guard let keep = SkinTempDisplay.dominantKind(valuesAscendingByDay: mixed) else { + return XCTFail("a non-empty window must have a kind") + } + let kept = mixed.filter { SkinTempDisplay.kind(of: $0) == keep } + XCTAssertEqual(kept, [-0.24, 0.21, 0.01, 0.11, 0.0]) + XCTAssertTrue(kept.allSatisfy { SkinTempDisplay.kind(of: $0) == keep }) + // The defect: unfiltered, the mean of a should-be-near-zero deviation window clears a degree + // and then reads BELOW 20, so it gets labelled Δ°C — plausible-looking and wrong. + let unfilteredMean = mixed.reduce(0, +) / Double(mixed.count) + XCTAssertGreaterThan(unfilteredMean, 1.0) + XCTAssertEqual(SkinTempDisplay.kind(of: unfilteredMean), .deviation) + let filteredMean = kept.reduce(0, +) / Double(kept.count) + XCTAssertLessThan(abs(filteredMean), 0.3) + } } diff --git a/Strand/Data/Repository.swift b/Strand/Data/Repository.swift index eeff3df288..a9411685fd 100644 --- a/Strand/Data/Repository.swift +++ b/Strand/Data/Repository.swift @@ -2187,7 +2187,16 @@ final class Repository: ObservableObject { for p in (try? await store.metricSeries(deviceId: id, key: key, from: from, to: to)) ?? [] { byDay[p.day] = p.value } } - return byDay.sorted { $0.key < $1.key }.map { (day: $0.key, value: $0.value) } + let merged = byDay.sorted { $0.key < $1.key }.map { (day: $0.key, value: $0.value) } + // #1705: skin_temp is bimodal — layer 1 (a CSV import) writes ABSOLUTE °C into the same key + // layers 2/3 fill with a baseline DEVIATION, so one window can hold both. Every consumer of + // this array aggregates it (Explorer's min/max/mean and its window-over-window delta, the + // correlation scan), and those are arithmetic across two scales. Reduce to the newest entry's + // kind HERE, at the one place the layers are merged, rather than at each screen. + guard key == "skin_temp", + let keep = SkinTempDisplay.dominantKind(valuesAscendingByDay: merged.map(\.value)) + else { return merged } + return merged.filter { SkinTempDisplay.kind(of: $0.value) == keep } } /// Every catalog metric's Explore series at once, memoized — the cross-catalog scan behind the diff --git a/android/app/src/main/java/com/noop/analytics/SkinTempDisplay.kt b/android/app/src/main/java/com/noop/analytics/SkinTempDisplay.kt index d6564aff5d..8d222d02de 100644 --- a/android/app/src/main/java/com/noop/analytics/SkinTempDisplay.kt +++ b/android/app/src/main/java/com/noop/analytics/SkinTempDisplay.kt @@ -19,6 +19,27 @@ object SkinTempDisplay { DEVIATION, } + /** + * The one kind a MIXED series must be reduced to before any aggregate is taken (#1705). + * + * A CSV import writes absolute °C and the computed pipeline writes a deviation, both into this + * same field, so one window can hold both. Formatting stays honest per value — [kind] is + * re-derived for each — but a min, max, mean or window-over-window delta drawn across both is + * arithmetic on two different scales, and it looks plausible: a handful of ~34 °C readings lift a + * should-be-near-zero deviation average past a full degree, and the mean is then itself below 20 + * so it gets labelled Δ°C. + * + * The newest entry decides, matching what `HealthVitalsLogic` already does for its sparkline — the + * reading the user is actually looking at sets the scale, and older entries of the other kind drop + * out rather than being converted, because converting needs a baseline that a fresh import-only + * install does not have. + * + * @param valuesAscendingByDay the window's values, ascending by day. + * @return the kind to keep, or null for an empty window. + */ + fun dominantKind(valuesAscendingByDay: List): Kind? = + valuesAscendingByDay.lastOrNull()?.let { kind(it) } + fun kind(value: Double): Kind = if (VitalBands.isAbsoluteSkinTemp(value)) Kind.ABSOLUTE else Kind.DEVIATION diff --git a/android/app/src/main/java/com/noop/data/WhoopRepository.kt b/android/app/src/main/java/com/noop/data/WhoopRepository.kt index 80b7b2dea4..b4b92d35a5 100644 --- a/android/app/src/main/java/com/noop/data/WhoopRepository.kt +++ b/android/app/src/main/java/com/noop/data/WhoopRepository.kt @@ -1823,7 +1823,24 @@ class WhoopRepository( // Book / any resolver read agrees with the mergeDaily dashboards instead of re-surfacing the // schedule target the merge already rejects. val perCandidate = candidates.map { it to resolvedRows(it, from, to) } - return MetricSeriesResolution(preferredSource, candidates, resolveFirstWins(perCandidate)) + val points = resolveFirstWins(perCandidate) + // #1705: skin_temp is bimodal — a CSV import writes ABSOLUTE °C into skinTempDevC while the + // computed pipeline writes a baseline DEVIATION, and the candidate list unions the imported and + // computed identities, so first-wins can resolve older days to one kind and recent days to the + // other. Every consumer aggregates these points (Compare's min/max and its min-max chart + // normalisation, Lab Book), and that is arithmetic across two scales. Reduce to the newest + // entry's kind HERE, at the one place the candidates are resolved, rather than at each screen. + if (key == "skin_temp") { + val keep = com.noop.analytics.SkinTempDisplay.dominantKind(points.map { it.value }) + if (keep != null) { + return MetricSeriesResolution( + preferredSource, + candidates, + points.filter { com.noop.analytics.SkinTempDisplay.kind(it.value) == keep }, + ) + } + } + return MetricSeriesResolution(preferredSource, candidates, points) } /** diff --git a/android/app/src/test/java/com/noop/analytics/SkinTempDisplayTest.kt b/android/app/src/test/java/com/noop/analytics/SkinTempDisplayTest.kt index 53d97ca1af..450ec4e40a 100644 --- a/android/app/src/test/java/com/noop/analytics/SkinTempDisplayTest.kt +++ b/android/app/src/test/java/com/noop/analytics/SkinTempDisplayTest.kt @@ -1,6 +1,7 @@ package com.noop.analytics import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Test @@ -73,4 +74,36 @@ class SkinTempDisplayTest { ) } } -} + + // ---- dominantKind (#1705) ---- + + @Test fun dominantKindIsTheNewestEntrys() { + assertEquals(SkinTempDisplay.Kind.DEVIATION, SkinTempDisplay.dominantKind(listOf(34.6, 35.1, -0.2))) + assertEquals(SkinTempDisplay.Kind.ABSOLUTE, SkinTempDisplay.dominantKind(listOf(-0.2, 0.1, 34.6))) + assertNull(SkinTempDisplay.dominantKind(emptyList())) + } + + @Test fun dominantKindOfASingleKindWindowIsThatKind() { + assertEquals(SkinTempDisplay.Kind.DEVIATION, SkinTempDisplay.dominantKind(listOf(-0.24, 0.21, 0.01))) + assertEquals(SkinTempDisplay.Kind.ABSOLUTE, SkinTempDisplay.dominantKind(listOf(32.39, 34.60, 36.64))) + } + + /** + * The regression guard the issue asked for: filtering by [SkinTempDisplay.dominantKind] must leave a + * window that no aggregate can straddle. Values are the reported ones — a 313-row absolute import + * (32.39..36.64, mean 34.60) coexisting with computed deviations inside +/-0.3. + */ + @Test fun filteringByDominantKindLeavesOneScale() { + val mixed = listOf(34.60, 35.12, 32.39, 36.64, -0.24, 0.21, 0.01, 0.11, 0.0) + val keep = SkinTempDisplay.dominantKind(mixed)!! + val kept = mixed.filter { SkinTempDisplay.kind(it) == keep } + assertEquals(listOf(-0.24, 0.21, 0.01, 0.11, 0.0), kept) + assertTrue(kept.all { SkinTempDisplay.kind(it) == keep }) + // The defect: unfiltered, the mean of a should-be-near-zero deviation window clears a degree and + // then reads BELOW 20, so it gets labelled deviation — plausible-looking and wrong. + val unfilteredMean = mixed.average() + assertTrue(unfilteredMean > 1.0) + assertEquals(SkinTempDisplay.Kind.DEVIATION, SkinTempDisplay.kind(unfilteredMean)) + assertTrue(kotlin.math.abs(kept.average()) < 0.3) + } +} \ No newline at end of file From ccea3a2fb4f240855f24e1858f5c233f4f5e5368 Mon Sep 17 00:00:00 2001 From: Fanboynz Date: Fri, 28 Aug 2026 21:25:07 +1200 Subject: [PATCH 2/2] analytics: apply the skin-temp reduction on both exploreSeries return paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-review of the reduction. It was conditional on a check that happens to always pass. exploreSeries opens with `guard source == "my-whoop" else { return await series(...) }`, and the filter sat only on the other return path, so the delegating one still returned a mixed window. It is unreachable today because MetricCatalog gives the only skin_temp descriptor the my-whoop source — but that makes the guard correct only because of a fact in a different file, which is how the original bug happened. Both paths now go through one helper, so "exploreSeries never returns a mixed skin_temp window" holds by construction rather than by coincidence. Verified rather than assumed while writing the helper's doc: series() really is ascending by day on both its branches (ORDER BY day ASC in MetricSeriesStore, and unionMetricSeries sorts), which is what dominantKind relies on; and exploreAllSeries does delegate per descriptor to exploreSeries, so the correlation scan inherits this too. Repository.swift is app-target, so the helper was compile-checked standalone first — including the labeled-tuple key path, which is the one bit of syntax here worth not assuming. Kotlin untouched: resolvedSeries has no analogous bypass, so both platforms now hold the same unconditional invariant. --- Strand/Data/Repository.swift | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/Strand/Data/Repository.swift b/Strand/Data/Repository.swift index a9411685fd..5bd913a6b3 100644 --- a/Strand/Data/Repository.swift +++ b/Strand/Data/Repository.swift @@ -2160,7 +2160,9 @@ final class Repository: ObservableObject { /// byte-identical. The flag is forwarded to the non-strap `series(...)` delegation below so every source /// path honours it. func exploreSeries(key: String, source: String, days: Int = 4000, fullHistory: Bool = false) async -> [(day: String, value: Double)] { - guard source == "my-whoop" else { return await series(key: key, source: source, days: days, fullHistory: fullHistory) } + guard source == "my-whoop" else { + return Self.oneSkinTempScale(key: key, await series(key: key, source: source, days: days, fullHistory: fullHistory)) + } guard let store = await ensureStore() else { return [] } let now = Date() let from = fullHistory ? "0000-01-01" : Self.dayString(now.addingTimeInterval(-Double(days) * 86_400)) @@ -2187,16 +2189,29 @@ final class Repository: ObservableObject { for p in (try? await store.metricSeries(deviceId: id, key: key, from: from, to: to)) ?? [] { byDay[p.day] = p.value } } - let merged = byDay.sorted { $0.key < $1.key }.map { (day: $0.key, value: $0.value) } - // #1705: skin_temp is bimodal — layer 1 (a CSV import) writes ABSOLUTE °C into the same key - // layers 2/3 fill with a baseline DEVIATION, so one window can hold both. Every consumer of - // this array aggregates it (Explorer's min/max/mean and its window-over-window delta, the - // correlation scan), and those are arithmetic across two scales. Reduce to the newest entry's - // kind HERE, at the one place the layers are merged, rather than at each screen. + return Self.oneSkinTempScale(key: key, byDay.sorted { $0.key < $1.key }.map { (day: $0.key, value: $0.value) }) + } + + /// #1705: reduce a `skin_temp` window to a single scale; every other key passes through untouched. + /// + /// The key is bimodal — a CSV import writes ABSOLUTE °C where the computed layers write a baseline + /// DEVIATION, so one window can hold both. Every consumer of an Explore series aggregates it + /// (min/max/mean, the window-over-window delta, the correlation scan), and those are arithmetic + /// across two scales: on the reported account a few ~34 °C readings lifted a should-be-near-zero + /// average past a full degree, and the mean then read below 20 so it was labelled Δ°C. + /// + /// Applied on BOTH of `exploreSeries`' return paths, deliberately. Today the only `skin_temp` + /// descriptor is `my-whoop`, so the delegating path is unreachable for it — but a guard that is + /// correct only because of a fact in another file is how this bug happened in the first place. + /// `series(day:value:)` is ordered ascending by day, which `dominantKind` relies on. + private static func oneSkinTempScale( + key: String, + _ series: [(day: String, value: Double)] + ) -> [(day: String, value: Double)] { guard key == "skin_temp", - let keep = SkinTempDisplay.dominantKind(valuesAscendingByDay: merged.map(\.value)) - else { return merged } - return merged.filter { SkinTempDisplay.kind(of: $0.value) == keep } + let keep = SkinTempDisplay.dominantKind(valuesAscendingByDay: series.map(\.value)) + else { return series } + return series.filter { SkinTempDisplay.kind(of: $0.value) == keep } } /// Every catalog metric's Explore series at once, memoized — the cross-catalog scan behind the