Repository navigation
Reject an out-of-domain strain denominator on both platforms - #1977
Merged
Merged
Conversation
…both platforms trimpToStrain evaluated 100 × ln(TRIMP+1) / ln(D) for any D. At D = 1 the division is by zero, so Swift returned +Inf while Kotlin's roundToLong() saturated to 9.223372036854776E16 — the same input, two different answers. Below 1 the sign flips, at or below 0 ln(D) is NaN. D ≤ 1 (and NaN) is now out of domain and scores 0 on both sides, like TRIMP ≤ 0. Kotlin additionally rounds in Double space, since roundToLong() still clipped a D just above 1 where Swift's .rounded() does not; above 2^53 a Double is already an integer, so the value passes through unchanged. The default D = 7201 and every fitted D (exp of a positive) are unaffected — no shipped Effort number moves. Mirrored regression tests cover D = 1, D below 1, NaN, valid denominators and the just-above-1 boundary; the Kotlin expected literals are the stdout of the Swift formula compiled standalone. Refs: issue #36 (fork)
…TRIMP Review follow-up on the log-map denominator domain, tests and docs only. The just-above-1 assertion compared bit-for-bit against a value derived from ln(1 + 2^-52) ~ 2.2e-16, where a 1-ulp libm difference (Darwin vs glibc, and CI runs the Swift job on macOS) moves the quotient by ~30 absolute. The case stays, now as isFinite plus a 1e4 tolerance, with the exact Linux-observed literal kept in the comment. Also pins the behaviour the Double-space rounding made identical: TRIMP = +inf scores +inf and TRIMP = NaN scores NaN on both platforms, confirmed against the standalone Swift oracle before asserting it. The domain guard is about D, not TRIMP. Doc comments now say the output is unbounded as D approaches 1 from above; the upper clamp is tracked separately (#12). Refs: issue #36 (fork)
Owner
|
Reviewed. The analysis is correct, the guard is right on both platforms, and the rounding change is the part I was most prepared to argue with and could not. Thank you @bhelm. What I verified rather than read:
A second divergence this quietly removesWorth stating, because you did not claim it and it makes the change stronger than advertised. Kotlin's One thing I would like changed before this mergesThe doc comments and test docs carry 11 bare issue references that mean something else in this repository: 9 occurrences of one number and 2 of the other. In this repository, number 36 is an old sleep-screen graphic report and number 12 is a CI pull request about APK artifact naming that landed long ago. Neither has anything to do with the log map. A reader who chases either one from Could you reword them to describe the thing instead of citing a number? For example "outside the map's domain" rather than the same phrase with a bare number after it, and "an upper clamp is tracked separately" rather than naming a number. The reference already in that file to the weightlifting accuracy issue is a genuine one for this repository and should stay exactly as it is. The behaviour note in the PR description is exactly right and I would like it kept for the merge message, so the non-finite TRIMP change is on the record. One suggestion, not blocking
One sentence on the comment would keep a future reader from "simplifying" it. Everything else looks good. Reword the issue references and I will merge it. |
…ssue numbers Reword the doc comments and test docs so they name the thing (outside the map's domain, an upper clamp tracked separately) rather than fork issue numbers that mean something else upstream. Add one sentence explaining why the abs(scaled) < 2^53 comparison must keep its direction: NaN takes the pass-through branch on purpose, and reversing it would collapse NaN to 0.0 through roundToLong() and break parity with Swift.
Author
|
Done: the eleven bare issue references are reworded to describe the thing ( |
9 tasks done
DX23876
added a commit
to DX23876/noop
that referenced
this pull request
Sep 16, 2026
Thirty real conflicts. The Android tree is dropped again per FORK_GUIDE; every other hunk was resolved by asking which mechanism this fork actually runs, not by preferring a side. Taken from upstream: • Protocol: ryanbr#2017/ryanbr#2018/ryanbr#2020 — the 5/MG historical optical and raw-IMU layouts, and the v26 window's ABSOLUTE base code beside its deltas. Its migration lands as `v57-ppg-waveform-base-code` (upstream's v44), since this fork's sequence is nine ahead of theirs. • BLE diagnostics ryanbr#1998-ryanbr#2003: a bond end that says WHY, named BLE integers, a session-budgeted hex dump, and "we hold the link" only when the OS says so. • Battery: ryanbr#1951 stop asking a 5/MG for a reading it refuses; ryanbr#1959 poll 0x2A19 for an unbonded one. • Scoring: ryanbr#1978/ryanbr#1982/ryanbr#1990 the RHR/HRV baseline gates, ryanbr#1977 the strain denominator's domain, RMSSD withheld on an over-counted night, ryanbr#1943 the resting-HR bin gate, the closed final HR/HRV window, HR-recovery requiring one CONTINUOUS effort, ryanbr#1984 nights bridged under minSleepMin, ryanbr#2015 motion re-folded only when that day's gravity moved, ryanbr#2016 a provisional Rest. • Charts ryanbr#2004/ryanbr#2005/ryanbr#2007/ryanbr#2009/ryanbr#2011 and ryanbr#1995's hero-ring detail route, ryanbr#1164's "Pending sync" caption under Rest. • Widgets: the heart-rate widget with its trace (ryanbr#1957/ryanbr#1964) and the widget cost counters. • Health: ryanbr#1503 — the Apple Health external UUID drops the strap id, so a re-pair no longer strands every prior record, plus the one-off sweep that reclaims the stranded ones; ryanbr#1205 GPS routes read back from Apple Health. • Storage: rolling retention for ppgWaveformSample, the skin-temp absolute backfill outside the 21-night window, per-table byte estimates. • Units split (body vs exercise distance), per-day wake overrides, the wind-down nudge pinned to its own day, and the German Sie→du conversion (29 entries, merged per LOCALE so the fork's own de edits stayed). Kept on the fork's side, with the reason: • ryanbr#1005's in-memory `dayScanCache` stays out, again: the persisted `DayScanFingerprint` survives relaunch and its key folds the per-day weight and max HR upstream's does not. • The steps-motion cache stays PERSISTED for the same reason. Upstream's failed-read-vs-empty-day distinction IS adopted into it — on a persisted cache a stale zero from a read error would outlive the process that made it. • Every stream read in the day loop keeps `limit: Int.max`. All five are uncapped together; taking StreamReadCap for three of them would leave the loop half-capped, which is worse than either choice. • The coach engine. Upstream's K1-K15 layer is not adopted: its single 40-row `coachMessage` table, its tool-less pre-baked context and its own brief are behind what this fork already ships. Its ADDITIVE half is kept — the on-device voice input, the Siri "Ask Coach" intent, the brief scheduler and brief widget, and streaming for the OpenAI-compatible providers this fork's Anthropic-only StreamingToolClient does not cover. They are wired up in the next commit; `CoachBriefWidget` is deliberately not registered until then. • Liquid's `DomainTheme` labels, value-sampled Charge tint, `LiquidScoreGauge` and `TodayCardSurface`; classic Today's ring order and `LiveEffort.today`. • RootTabView's four-tab shell and `MoreCatalog`: Coach hangs on content here, not on a tab. Two duplicate mechanisms collapsed rather than carried: • `ChargeBreakdownWiring` (upstream) and `ChargeBreakdownFormat` (fork) are the same extraction done twice. The fork's survives, Coupled now reads it, and upstream's TEST is kept — it is coverage this path never had. • Upstream's `CoachMessageStore` is dropped; `CoachConversationStore` already autosaves the whole multi-conversation history. One defect found while resolving, fixed here: `LiveEffort.today` read HR with no explicit limit, so `hrSamples`' 8000-row default truncated it — at ~18k rows a day that covered about ten hours and the live Effort silently stopped climbing. It failed safe (`effectiveEffort` takes the max), which is why it went unseen. `Live5RestFrozenTests` is removed by upstream, which replaces it with `Live5NoGravityRestChainTests`. `local_day_windows_oracle.json` moves out of upstream's android path into `Tests/StrandAnalyticsTests/Oracles/` — NOT `Fixtures/`, which this repo's .gitignore excludes everywhere. Verified: StrandAnalytics 2233, WhoopStore 627, WhoopProtocol 700, StrandImport 293, StrandDesign 120, NoopLocalAccess 14, SemanticMemory 19, OuraProtocol 209 and PolarProtocol 25 all green; macOS and iOS both build; doc_comment_lint clean; `i18n_audit --ci` byte-identical to its pre-merge output (it is red on main for reasons that predate this, and the seven new widget literals were given all nine locales so the gate did not get worse). Six StrandTests were ALREADY failing on main before this merge and still are — `CoachMyLogsAndZonesTests` (the tool census pin was not bumped when the Hevy tools landed) and five in `RawPhysiologyUnionTests`. Every input to them — CoachTools.swift, ToolConsent.swift, Repository.swift and both test files — is byte-identical to main, so this merge neither caused nor fixed them. They went unnoticed because app-build.yml last ran on 2026-08-25. Analysis migration required: YES. The scoring changes above make stored derived values stale; the recipe bump and its resumable migration are the next commit.
DX23876
added a commit
to DX23876/noop
that referenced
this pull request
Sep 16, 2026
The 2026-09-09 sync moves scored VALUES, not just how they are presented, so a `dailyMetric` row computed under v2 no longer means what the current code would produce from the same inputs. Nine changes do it, each on some subset of nights: the RHR and HRV baseline gates (ryanbr#1978/ryanbr#1982/ryanbr#1990), the strain denominator's domain check (ryanbr#1977), RMSSD withheld on a night whose R-R over-counts, the ryanbr#1943 resting-HR bin gate, the closed final HR/HRV window, heart-rate recovery requiring one CONTINUOUS effort, ryanbr#1984's bridged fragmented nights, and ryanbr#2015's gravity-conditional motion re-fold. So: `Analysis migration required: yes`. `currentAnalysisRecipeVersion` 2 → 3. No new migration machinery — the existing one already has the properties CLAUDE.md asks for. `prepareAnalysisRecipe` reads the cursor, sees `.migrate(2, 3)` and runs `runAnalysisMaintenance`, which is a forced `analyzeRecent(maxDays: 21, allowDayReuse: false)` that marks the recipe cursor ONLY after the pass returns, so an interrupted or failed run re-runs rather than silently claiming the rescore. It re-derives; it deletes no raw samples and no user sleep or workout corrections. 21 nights is the narrowest interval provable rather than a round number: every one of the nine is a per-night derivation, and the engine's own scoring window is 21 nights. Older nights keep their v2 values until re-scored for another reason — the same contract v1 → v2 shipped with. Deliberately NOT in this bump: the skin-temp absolute backfill for nights OUTSIDE that window. It is its own resumable walker, fill-only on a NULL column, reached on purpose from the Test Centre rather than on launch, so folding it in would widen the migration to the whole library for a value the rescore does not compute anyway. The decision tests are extended where they were weakest: the three existing cases are written against `current`, so they stay green THROUGH a bump without ever witnessing one. Two literal pins are added — that a v2 install migrates as `2 → 3`, and that an install already at 3 (or rolled back from a 4) never rescores on relaunch, which is the failure mode using a marketing or build number here would cause and which is invisible until someone's phone spends twenty minutes re-scoring after a cosmetic update. The first pin is MEANT to fail on the next bump: that failure is the prompt to answer the migration question for whatever the bump carries. Verified: StrandTests 2950, 0 failures; all nine packages green; macOS and iOS both build; doc_comment_lint clean. Recorded in docs/fork/decisions.md. Analysis migration required: yes (this commit is it).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
trimpToStrainmaps TRIMP onto [0, 100] withmaxStrain * log(trimp + 1) / log(D), but neverchecks that
Dis inside the map's domain. AtD == 1the divisor isln(1) = 0: Swift returns+Inf, while Kotlin's(value * 100).roundToLong() / 100.0saturates at theLongceiling andreturns
9.223372036854776E16for the same input. Below 1 the sign flips; at or below 0 the resultis NaN. Two platforms, one input, two different numbers — reported as bhelm#36.
Change
Packages/StrandAnalytics/Sources/StrandAnalytics/StrainScorer.swift:guard denominator > 1 else { return 0 }after the existingtrimp <= 0gate. NaN fails the predicate too.android/app/src/main/java/com/noop/analytics/StrainScorer.kt: the identical guard(
if (!(denominator > 1)) return 0.0), plus rounding moved into the Double domain —if (abs(scaled) < 2^53) scaled.roundToLong().toDouble() else scaled. The guard alone still leftD = 1.0000000000000002diverging (Swift3.12e17vs the clipped9.22e16), the samesaturation the issue names. Below 2^53 the new expression is bit-for-bit what
roundToLong()did; at or above 2^53 every Double is already integral, so the pass-through is the rounded
value.
StrainDenominatorDomainTests.swift,StrainScorerDenominatorDomainTest.kt)pinning the same out-of-domain cases, valid cases, boundary literal and non-finite
trimppropagation.
Verification
swiftc -O main.swift -o t && ./t), and its stdout pasted into the Kotlin test's KDoc as the expected literals(
1|7201 → 7.7999999999999998,7200|7201 → 100,1|2 → 100, allD <= 1and NaN →0). TheSwift test pins the same values, so the guard is held in both directions.
swift test --filter StrainDenominatorDomainTests→ 3 tests, 7 failures (D = 1.0→inf,D = 0.5→-100.0,D = -3.0→-nan,D = nan→nan)../gradlew testFullDebugUnitTest --tests "com.noop.analytics.StrainScorerDenominatorDomainTest"→ 3 tests,2 failed.
cd Packages/StrandAnalytics && swift test --filter Strain→ 46 tests, 0failures.
./gradlew testFullDebugUnitTest --tests "com.noop.analytics.Strain*"→ 19 tests,0 failures, 0 errors; the wider Kotlin run including
BanisterParityOracleTestandChargeEffortRestScoringTestis 54 tests green.python3 Tools/doc_comment_lint.py→OK no NEW detached doc comments.i18n_audit.pynotapplicable (no user-facing strings).
1e4tolerance rather than bit equality: the expected valuedivides by
ln(1 + 2^-52) ≈ 2.22e-16, so a 1-ulp difference inlogbetween glibc (where theoracle ran) and Darwin libm (where
swift-packages.ymlruns) lands as ~±30 absolute. The exactobserved Linux literal stays in the comment as the centre.
StrandTests— no macOS/Xcode available to me. Neitherchanged source is app-target code, so
swift-packages.ymlandandroid.ymldo cover this diff.No hardware involved; this is pure analytics.
Behaviour change / user-visible effects
No shipped number moves. The only caller on each platform is
strain(…), whose resolveddenominator is
7201or the Banister ceiling + 1 (≈6175 / ≈5300) — far above 1 and far below 2^53,so they take the identical rounding path. There is no app-layer or preference-supplied denominator
override in the tree; the
denominator:parameter is unused outside tests. Existing goldens(51.96 / 69.99 / 77.78 / 92.20 / 100.0) are unchanged.
The one real behaviour change is on the public API: a non-finite
trimpon Android now propagates(
+Inf→+Inf,NaN→NaN) instead of collapsing to9.2e16/0.0. That makes Kotlin matchSwift, and it is pinned on both sides.
Scope limits and follow-ups
D → 1⁺, tracked as[P2] N-07: Strain integration miscounts edge samples and can exceed 100 bhelm/noop#12 and now noted in both doc comments.
sampleDurationsMinutes(same file, also CI: rename Android APK to NOOP-android-v<VER>.apk (consistent artifact naming) #12)is untouched.
D = +Infwas already identical (divides to0.0) and was left alone.repository — nothing under
Tools/referencestrimpToStrain— so it could not be run here. Thein-repo substitutes actually run are
BanisterParityOracleTestand the new mirrored oracle pair.Branch:
fix/issue-36-strain-denominator-domain. Contribution offered under the repository'sPolyForm Noncommercial 1.0.0 license.