Strain: weight each HR sample by its own gap, not the window's first one - #963
Merged
Conversation
…one (#950) TRIMP inferred ONE per-sample duration from the first two timestamps and multiplied the whole zone-weight sum by it. NOOP's HR stream is not uniformly spaced — live Bluetooth ~1 s, banked 5/MG history ~30 s, dropouts larger — so whichever gap happened to come first set the scale for the entire window. A workout window and the day containing it start at different samples, picked different factors, and produced Effort numbers that could not be compared: the #950 ride scored 6.9 while its own day scored 14. Each reading now carries the gap to its successor, clamped to 2 min so a dropout hole cannot be credited to the reading before it (one zone-5 sample could otherwise invent hours of effort). The clamp is 4x the sparsest real cadence, so nothing genuine is truncated. For a uniformly spaced series every gap equals the first gap, so results are byte-identical to the old code — no existing test moves, which is the point: this only changes windows whose spacing varies, which are exactly the broken ones. Both platforms in lockstep; twin test files with the same fixtures. Modelled old-vs-new before writing it: uniform hour identical (240 TRIMP), the reported mixed-cadence shape recovers 25.5 -> 61.9 Effort, and the 3 h dropout pair is capped at 16 TRIMP instead of 1440. The second defect found in #950 — ManualWorkoutRescore never passing the wearer's resting HR, so workouts score against a hardcoded 60 — is deliberately NOT here; it needs the resting HR threaded through IntelligenceEngine and is its own concern.
Two HrSample types exist (com.noop.data vs com.noop.protocol) and the test named neither, so the unit-test compile failed. StrainScorer and the sibling strain tests use the data one. Caught by Android CI; my local partial compile never built the test file, only the source.
…950) The second defect behind #950. The day's Effort passes the wearer's measured resting HR into the %HRR denominator; the workout rescore always took StrainScorer's default 60 — and so did its calories call, whose fourth argument was a literal null. For a fit wearer the real reserve is wider, every sample sits lower in the zone table than it should, and the workout under-scores relative to the very day it sits in. ManualWorkoutRescore.scored gains restingHR (null = old behaviour, byte for byte), threaded to both the strain and the Keytel calories model. The engine derives it from the days it just scored — the most recent Computed with a measured rhr — identically on both platforms, so a cold start with no measured resting keeps the old default rather than inventing one. The display-fill and live-session paths still use the default; they are transient renderings, and fixing them needs a resting source threaded through app-target code on both platforms — a follow-up, not a rider on this PR. Twin tests pin the seam: 148 bpm is chosen because the Edwards zone genuinely flips with the reserve (67.7% of a 60-resting reserve = zone 2; 71.0% of a 45-resting one = zone 3) — the first fixture, 145 bpm, sat in zone 2 under BOTH restings and would have passed vacuously as an equality. null-resting is pinned byte-identical, and the kcal movement proves the calories model sees the same value.
…n rate Both platforms failed the same new assertion with identical kcal under both restings, and they were right to: in estimateBoutCalories the resting HR only sets activeThreshold (resting + 30% HRR). An all-148bpm window is active under either resting, so kcal cannot move. The test now mixes in a 95 bpm warm-up, which is active under a 45-resting threshold (88.5) and resting-rate under a 60-resting one (99) — the mechanism the model actually has. The scored() doc comments overstated the same thing and are corrected.
The doc claimed it was kept for callers wanting one representative figure; no such caller exists. Its only consumer is the uniform-identity test, which uses it to compare the new accumulation against the shipped old formula rather than a reimplementation. Said so, with a deletion note.
Re-review catch, and it was my own comment that gave it away: I wrote "the most recent scored day" and then used lastOrNull / last(where:). `out` is NEWEST-FIRST. Both engines score with `for offset in 0..<maxDays` and `dayStart = nowLocalMidnight - offset * 86_400`, counting backwards from today, so out[0] is today and the tail is the oldest day in the window. Taking the last match scored today's workout against a resting HR up to maxDays old. Wrong identically on both platforms, so parity held while the intent did not — which is the failure mode a byte-parity contract cannot catch on its own. No test added deliberately: the ordering is a property of a day loop 300 lines away, so a unit test over a projection would pin my helper rather than the risk (someone reversing that loop). The comment names the loop and its direction so the next reader can check it without re-deriving it.
That half is confirmed against #950's export (rhr floor=50); this one is a real defect whose impact on the reporter I can no longer claim — his strap banks at 0.997 records/second, so the first-gap guess was probably right for his ride. Two independent analytics changes with different evidence and different risk should not ride together. Moved to #972.
ryanbr
added a commit
that referenced
this pull request
Jul 30, 2026
…950) (#972) Split out of #963 so it stands on its own: this half is confirmed against the reporter's data, the sample-duration half is not. The day's Effort passes the wearer's measured resting HR into the %HRR denominator; the workout rescore always took StrainScorer's default 60, and so did its calories call. For a fit wearer the real reserve is wider, every sample sits lower in the zone table than it should, and the workout under-scores relative to the very day it sits in. Confirmed on #950's export: `rhr day=2026-07-30 floor=50`. A 10 bpm error in the denominator, on a real user. scored() gains restingHR (null = old behaviour, byte for byte), threaded into both the strain and the calories active-threshold. The engine derives it from the days it just scored — the FIRST Computed with a measured rhr, because `out` is newest-first (both engines count backwards from today) — identically on both platforms. Display-fill and live-session paths still use the default; they are transient renderings and need a resting source threaded through app-target code, which is a separate change.
This was referenced Jul 31, 2026
ryanbr
added a commit
that referenced
this pull request
Jul 31, 2026
….2.2/304 (#998) main has been on 9.2.1 for 42 commits. That was invisible until the testing build cut from main stamped 9.2.1 / versionCode 303, and Android refused to install it over the 29 July staging build (9.2.2 / versionCode 304) — a downgrade, surfaced as the unhelpful "App not installed". versionCode goes to 305 rather than 304 because 304 is already published on the rolling testing-latest tag; reusing it would leave two different builds sharing a code. MARKETING_VERSION and versionName move together per CLAUDE.md. No release notes here — this is the version bump only, and 9.2.2's notes still owe users the user-visible changes from the last 42 commits (Effort shifts from #963 and #992, the new Apple Health permission prompt, the Oura resting-HR heal that needs a re-sync, and the sleep-staging change from #987).
ryanbr
added a commit
that referenced
this pull request
Jul 31, 2026
Notes covering everything merged since v9.2.1 (43 PRs), the in-app What's
New entry generated for both platforms, and the version bumps.
- docs/releases/v9.3.0.md — release notes plus the whatsnew front-matter,
with title translations for all five locales so the i18n gate has no
English fallback to warn about.
- AppChangelog.{kt,swift} + six strings.xml — generated by
Tools/appchangelog-gen.py, not hand-written, so the two platforms carry
byte-identical items.
- CHANGELOG.md — a 9.3.0 section in the existing house format.
- MARKETING_VERSION 9.2.2 -> 9.3.0, versionName 9.2.2 -> 9.3.0,
versionCode 305 -> 306.
The notes lead with a 'Scores that change' section because three merges
move numbers users have already seen — gap-weighted strain (#963), the
workout resting-HR fix (#983), and the sleep transition rule (#348) — and
the Oura fix (#375) corrects the importer, not rows already written, so it
needs a re-import to take effect.
ryanbr
added a commit
that referenced
this pull request
Aug 1, 2026
Effort was resolved independently in several places and only the hero ring knew about the live in-progress value. The Key Metrics tile and the HR chart's edge badge read DailyMetric.strain straight off the daily row, which is rewritten only when the heavy daily pass runs. The reporter's morning showed 2.3 on the ring and 0.5 in both of the others, with the HR trace climbing from about 09:00 and the screenshot taken at 12:22 — the row predated the whole morning. Recovery and Rest matched everywhere precisely because neither has a live path. Not a 9.3.0 regression: the live hero landed in v4.0.1 on 15 June and the tile has read the stored column unchanged since 14 June. #963 made Effort track an active morning more closely, which widened the gap enough to see. StrainScorer.effectiveEffort(live:stored:) is now the one rule, Swift and Kotlin, with the twin test suites pinning it. The MAX in it is not a tie-break but the never-drop floor from #489/#506, where a sparse-HR live under-read replaced a real 38.3 with 0 — worth naming, because it is the part someone would 'simplify' into preferring live. Android: resolved once in TodayScreen and threaded to MetricGrid and HeartRateTrendCard, alongside the resolved values those already take (caloriesForDay, importedStepsForDay). The near-zero Effort caption had a fourth hand-rolled copy of the same expression; it now uses the shared one and keeps returning null for past days so the caption stays today-only. iOS already had effortStrain() as the single point, and its gauge and near-zero note used it; effortMarker and the Effort StatTile reached past it to the row. Both now resolve through it, and it delegates to the shared rule.
This was referenced Aug 1, 2026
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.
A real defect found while investigating #950. The resting-HR fix that #950 actually confirms is
#972 — split out so each half carries its own evidence.
Read this first: the reporter's export shows his strap banking at 0.997 records/second, essentially
uniform. So the first-gap guess was probably right for his ride, and this fix likely is not what cost
him the 17. It is a genuine bug on non-uniform windows — where it can both under- and over-count, badly —
but its impact on real users is unquantified, and I would rather say so than let the #950 framing
carry it.
The bug
TRIMP guessed ONE per-sample duration from the window's first two timestamps and multiplied the whole
zone-weight sum by it. NOOP's HR stream isn't uniformly spaced — live Bluetooth ~1 s, banked 5/MG history
~30 s, dropouts larger — so whichever gap came first set the scale for the entire window. The workout
window and the day containing it start at different samples, picked different factors, and produced
Effort numbers that can't be compared. That's the report: a hard ride at 6.9 inside a day at 14.
The fix
Each reading carries the gap to its successor, clamped to 2 min so a dropout hole is never credited
to the reading before it (one zone-5 sample could otherwise invent hours of effort; unclamped, a 3 h gap
pair scores 1440 TRIMP — clamped, 16). The clamp is 4× the sparsest real cadence (~30 s, the #482 5/MG
path), so nothing genuine is truncated.
For uniformly spaced samples every gap equals the first gap, so results are identical to the old code
after the 2 dp strain rounding. Stated precisely after re-review: the raw TRIMP can differ at the ULP
level, because the old code multiplied the summed weights once while the new one rounds per term —
(Σw)·dvsΣ(w·d)differ in the 13th decimal on a 1 Hz cadence. It cannot survivetrimpToStrain's2 dp rounding, and CI proves it: every pre-existing strain assertion on both platforms passed unchanged.
Cross-platform Swift↔Kotlin parity is exact — both now accumulate in the same order.
The design has in-tree precedent I only found re-reviewing:
estimateBoutCaloriesalready weightseach sample by its own gap, clamped (
min(gap, mergeGapS)). Calories were never wrong — only strain usedthe single-factor shortcut. The clamps differ (150 s there, 120 s here — this one chosen as 4× the
sparsest real cadence); unifying them is possible but each is documented on its own reasoning.
Modelled old-vs-new before writing it:
What the clamp costs on a gappy day (re-review, quantified)
The framing above is mixed cadence, but the change has a second consequence I had not measured: wear
gaps. Old code effectively ignored them — every sample carried the first gap regardless — while each
sample before a hole now carries up to
maxSampleGapMin. So a gappy day gains a little Effort.Modelled on a 8 h day at 1 Hz, zone 1, with 20-minute gaps, against the reporter's own resting HR of 50:
The ceiling is 2 min × zone weight per gap, and the log map compresses it hard, so even a pathological
40-gap day moves about a third of a point on the scale users see. Worth knowing it is non-zero and always
upward; not worth tuning the clamp for.
Scope
Both platforms in lockstep, twin test files with the same fixtures: uniform-is-identical, the reported
mixed-cadence shape, day ≥ its own workout, the clamp, 30 s cadence not clamped, and the old edge-case
fallbacks. No strings, no schema, no BLE.
The second #950 defect is now here too
ManualWorkoutRescorescored every workout against a hardcoded resting HR of 60 — strain's %HRRdenominator and the calories model's active threshold both took the default — while the day total used
the wearer's measured resting.
scored()gainsrestingHR(null = old behaviour, byte for byte), andthe engine threads the most recent scored day's measured
rhrinto the rescore, derived identically onboth platforms from the
Computeddays it just built.Still default-60, deliberately: the display-fill (
fillWorkoutHrFromStrap/ Repository'sstrainProfilefill) and the live-session paths. They are transient renderings, and threading a restingsource through app-target code on both platforms is a follow-up, not a rider.
Two of my test premises were wrong and CI caught both — worth recording since each was a test that
would have passed vacuously or asserted a mechanism that doesn't exist:
the 147–151 band, so it now uses 148;
(resting + 30% HRR), so an all-hard window produces IDENTICAL kcal — both platforms failed with the
same number, and the fixture now straddles the two thresholds (95 bpm warm-up).
Effect on stored data: scores recompute on the next analysis pass; historical Effort on
mixed-cadence days will change (upward). That is the fix working, but it is user-visible.