Rescore workouts against the measured resting HR, not a hardcoded 60 (#950) by ryanbr · Pull Request #972 · ryanbr/noop

ryanbr · 2026-07-30T17:00:54Z

Split out of #963 so each half stands on its own evidence. This one is confirmed against the reporter's
data; the sample-duration half in #963 is not.

The bug

The day's Effort passes the wearer's measured resting HR into the %HRR denominator. The workout rescore
never did — ManualWorkoutRescore.scored took StrainScorer's default of 60, and its calories call
passed null, which lands on the same default.

For a fit wearer the real reserve is wider, so every sample sits lower in the Edwards zone table than it
should, and the workout under-scores against the very day it sits in. That mismatch is the
incomparability #950 reports.

Confirmed on real data

#950's export: rhr day=2026-07-30 floor=50. His measured resting is 50; his workouts were scored
against 60. A 10 bpm error in the denominator, on a real user, not a modelled shape.

The change

scored() gains restingHR — null keeps the old behaviour byte for byte, so a cold start with no
measured resting is unchanged rather than inventing a number. It reaches both the strain denominator and
the calories active-threshold.

The engine derives it from the days it just scored: the first Computed carrying a measured rhr,
because out is newest-first — both engines run for offset in 0..<maxDays counting backwards from
today. Same derivation on both platforms.

Still default-60, deliberately: the display-fill and live-session paths. Those are transient
renderings, and threading a resting source through app-target code on both platforms is its own change.

Tests

Twin files, same fixtures. The strain fixture is 148 bpm specifically because the Edwards zone flips
with the reserve there (67.7% of a 60-resting reserve = zone 2; 71.0% of a 45-resting one = zone 3) — my
first attempt used 145, which sits in zone 2 under both restings and would have passed as a vacuous
equality.

The kcal test straddles the active threshold (a 95 bpm warm-up: active under a 45-resting threshold of
88.5, resting-rate under a 60-resting one of 99). My first version asserted the resting HR reaches the
burn rate; it doesn't, it only sets the threshold — and both platforms failed with identical kcal,
which is the parity contract doing its job.

null-resting is pinned byte-identical on both sides.

…950) 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.

…one (#963) * Strain: weight each HR sample by its own gap, not the window's first 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. * Import com.noop.data.HrSample in the new test 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. * Rescore workouts against the measured resting HR, not a hardcoded 60 (#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. * Fix the kcal test: resting HR gates the active threshold, not the burn 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. * Say why sampleDurationMinutes is kept, honestly 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. * Take the FRESHEST measured resting HR, not the oldest 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. * Split the resting-HR fix out into its own PR 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.

#983) Re-review catch. The fix was incomplete: two lines below the strain call, estimateBoutCalories was still being passed null/nil for resting HR — the same defect, in the same function, with the correct value already computed and in scope one line above. It matters for the same reason: the calories model's active-vs-resting threshold sits at resting + 30% HRR, so the hardcoded default silently shifts what counts as active. And #972 already threads it in the rescore path, so leaving it null here meant a saved workout's kcal disagreed with its own re-score exactly as its Effort did — this PR would have fixed one of the two and left the other, side by side. One argument per platform.

…#992) * Workouts: score the SAVED workout with the measured resting HR (#983) Replaces #990, cut back after two rounds of self-review each found a defect that PR had introduced. The bug is unchanged: the workout paths called StrainScorer.strain without a restingHR, so they silently took the hardcoded default of 60. %HRR is (bpm - resting) / (max - resting), so the default moves every zone boundary — at 136 bpm with maxHR 190 it is the difference between zone 1 and zone 2. Today's Effort and the manual rescore (#972) already thread the measured value, so a stored workout disagreed with its own re-score. This version fixes ONLY the saved workout, on both platforms: one read at save time, no new state, no lifecycle. #990 also threaded it into the live per-sample readout, which needs the value latched for the session — and that latch is where both of its bugs came from. First it re-read the denominator on every sample, so a mid-session sync could re-score the whole window and visibly step the number. Then, with the latch in place, endWorkout cleared it before the save, so the STORED number could differ from the one shown on screen. Neither was inherited; both were introduced by that PR. The live readout is a transient running estimate and stays on the default here. That means it can now differ from the saved figure at the end of a session — accepted deliberately, because the stored number is what history compares and what the rescore path already agrees with, and because a per-sample denominator has now been got wrong twice. Tests carried over from #990 unchanged: a parity pair pinning that resting HR moves the zone a sample lands in, and that it carries through to the score. * Workouts: thread the same resting HR into the saved workout's calories (#983) Re-review catch. The fix was incomplete: two lines below the strain call, estimateBoutCalories was still being passed null/nil for resting HR — the same defect, in the same function, with the correct value already computed and in scope one line above. It matters for the same reason: the calories model's active-vs-resting threshold sits at resting + 30% HRR, so the hardcoded default silently shifts what counts as active. And #972 already threads it in the rescore path, so leaving it null here meant a saved workout's kcal disagreed with its own re-score exactly as its Effort did — this PR would have fixed one of the two and left the other, side by side. One argument per platform.

…yanbr#950) (ryanbr#972) Split out of ryanbr#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 ryanbr#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.

…one (ryanbr#963) * Strain: weight each HR sample by its own gap, not the window's first one (ryanbr#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 ryanbr#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 ryanbr#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. * Import com.noop.data.HrSample in the new test 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. * Rescore workouts against the measured resting HR, not a hardcoded 60 (ryanbr#950) The second defect behind ryanbr#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. * Fix the kcal test: resting HR gates the active threshold, not the burn 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. * Say why sampleDurationMinutes is kept, honestly 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. * Take the FRESHEST measured resting HR, not the oldest 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. * Split the resting-HR fix out into its own PR That half is confirmed against ryanbr#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 ryanbr#972.

…r#983) (ryanbr#992) * Workouts: score the SAVED workout with the measured resting HR (ryanbr#983) Replaces ryanbr#990, cut back after two rounds of self-review each found a defect that PR had introduced. The bug is unchanged: the workout paths called StrainScorer.strain without a restingHR, so they silently took the hardcoded default of 60. %HRR is (bpm - resting) / (max - resting), so the default moves every zone boundary — at 136 bpm with maxHR 190 it is the difference between zone 1 and zone 2. Today's Effort and the manual rescore (ryanbr#972) already thread the measured value, so a stored workout disagreed with its own re-score. This version fixes ONLY the saved workout, on both platforms: one read at save time, no new state, no lifecycle. ryanbr#990 also threaded it into the live per-sample readout, which needs the value latched for the session — and that latch is where both of its bugs came from. First it re-read the denominator on every sample, so a mid-session sync could re-score the whole window and visibly step the number. Then, with the latch in place, endWorkout cleared it before the save, so the STORED number could differ from the one shown on screen. Neither was inherited; both were introduced by that PR. The live readout is a transient running estimate and stays on the default here. That means it can now differ from the saved figure at the end of a session — accepted deliberately, because the stored number is what history compares and what the rescore path already agrees with, and because a per-sample denominator has now been got wrong twice. Tests carried over from ryanbr#990 unchanged: a parity pair pinning that resting HR moves the zone a sample lands in, and that it carries through to the score. * Workouts: thread the same resting HR into the saved workout's calories (ryanbr#983) Re-review catch. The fix was incomplete: two lines below the strain call, estimateBoutCalories was still being passed null/nil for resting HR — the same defect, in the same function, with the correct value already computed and in scope one line above. It matters for the same reason: the calories model's active-vs-resting threshold sits at resting + 30% HRR, so the hardcoded default silently shifts what counts as active. And ryanbr#972 already threads it in the rescore path, so leaving it null here meant a saved workout's kcal disagreed with its own re-score exactly as its Effort did — this PR would have fixed one of the two and left the other, side by side. One argument per platform.

ryanbr mentioned this pull request Jul 30, 2026

Strain: weight each HR sample by its own gap, not the window's first one #963

Merged

ryanbr merged commit bcf9d8e into main Jul 30, 2026
13 checks passed

ryanbr deleted the fix/workout-resting-hr branch July 30, 2026 17:12

This was referenced Jul 30, 2026

Workouts: score with the measured resting HR, not the default 60 (#983) #990

Closed

Workouts: score the SAVED workout with the measured resting HR (#983) #992

Merged

Navigation Menu

Repository navigation

Rescore workouts against the measured resting HR, not a hardcoded 60 (#950) - #972

Rescore workouts against the measured resting HR, not a hardcoded 60 (#950)#972
ryanbr merged 1 commit into
mainfrom
fix/workout-resting-hr

ryanbr commented Jul 30, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

ryanbr commented Jul 30, 2026

The bug

Confirmed on real data

The change

Tests

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant