name the sample gap cap so both sides of a bout can read it - #42
Merged
Conversation
The active-EE estimator has always run Keytel 2005's age/mass/sex model. The same paper publishes a second one that adds a VO2max term, and it is the more accurate of the pair: fitness is what decides how much energy a given heart rate represents, because a higher VO2max means a greater stroke volume, so the same beat moves more oxygen. Without that term the model has to substitute the derivation cohort's mean fitness for everybody, which reads high for untrained people and low for athletes. Each sex's coefficient block gains the fitness-adjusted constants, and activeKcalPerS picks the model per call. dailyEnergy and estimateBoutCalories both take an optional vo2max and thread it down. OPTIONAL EVERYWHERE. Omit it and every caller gets exactly the numbers it got before, byte for byte, which is what the two backward-compatibility tests pin. A non-positive or non-finite value is treated as absent rather than fed to the regression: VO2max is strictly positive, and 0 is this package's "not measured" shape, not a reading. VO2max belongs to the ACTIVE term only. Below the bout gate, and for the Mifflin/Harris-Benedict floors, there is no fitness term at all. Expected values in the tests are computed by hand from the published equations so they pin the arithmetic rather than the implementation.
|
Warning Review limit reached
Next review available in: 39 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe calorie API now exposes ChangesShared calorie merge-gap cap
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/onehz/calories_vo2max_test.dart`:
- Around line 148-158: Extend the test around Calories.dailyEnergy to verify
that invalid vo2max values—0, a negative number, double.nan, double.infinity,
and double.negativeInfinity—produce the same total and active figures as
omitting vo2max. Reuse the existing day, _male, and _hrMax inputs and preserve
the current null fallback assertion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 71edbb31-55e6-4d46-b606-e28c47ae6595
📒 Files selected for processing (2)
lib/src/onehz/workout/calories.darttest/onehz/calories_vo2max_test.dart
The guard rejects on three separate conditions — null, non-finite, and non-positive — and all three have to land on the same fallback. Only null was covered. Zero and negatives are "not measured" rather than readings, since VO2max is strictly positive. A non-finite value matters more: it would otherwise propagate straight through the linear term and persist a NaN/Infinity daily total, not just spoil one bout. Asserted with exact equality rather than closeTo, because a correct fallback runs the identical code path — anything short of a bit-identical answer means the value got through. Verified the cases have teeth by weakening the guard to `vo2max != null` and confirming both tests fail. Covers both entry points; the daily one also asserts the total stays finite.
…r with it The guard checked the sign and finiteness and nothing else, so the fitness term was a bare multiplication with no ceiling: a million through a ten-minute bout came back as 965,000 kcal. The value arriving there is not measured. It is usually a resting-HR ratio estimate, so it carries every artifact of the resting HR it divides by, and one bad night at 30 bpm against a 200 bpm HRmax reads as 102 mL/kg/min, which prices a bout 32% high and looks perfectly ordinary doing it. Unit confusion fails the same way at the other end. A value outside the range a human can occupy is rejected rather than clamped: it says nothing about this person's fitness, so the age/mass/sex model is the honest answer. Values inside the range but outside Keytel's cohort are still used — extrapolating a published linear model is not the same thing as a broken input. Both results now carry which of the two models priced them, for the reason usedDefaultHrmax already exists: a rejected anchor is otherwise indistinguishable from one that was never supplied. WorkoutDetect never got the parameter, so a detected bout and a hand-logged one over the same trace would have been scored on different models — about 30% apart, same app, same day. The nonbinary mean is only an identity while both blocks stay above the zero floor. The floor is applied after the mean block is evaluated, so a profile extreme enough to drive one sex negative breaks it. Pinned rather than reworded, since the alternative reading — averaging two clamped results — is not what the mean coefficient block means.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/src/onehz/workout/calories.dart`:
- Around line 300-304: Update the fitness-model reporting in
lib/src/onehz/workout/calories.dart at lines 300-304 and 375 so usedFitnessModel
is set only when an above-flex/above-threshold sample actually evaluates the
fitness-adjusted active regression; do not derive it solely from
usableVo2max(vo2max). Ensure fully resting bouts or days report the resting
calculation instead, with both affected sites following this behavior.
In `@lib/src/onehz/workout/workout_detect.dart`:
- Line 424: Update WorkoutDetector.detect and the ExerciseSession model to
preserve calorie-model metadata: add the caloriesUsedFitnessModel field,
populate it from cal.usedFitnessModel when constructing ExerciseSession, and
include it in ExerciseSession.toJson(). Extend the related tests to verify the
value is retained and serialized.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b12e911e-3684-42d8-bed9-2708b12f8a45
📒 Files selected for processing (3)
lib/src/onehz/workout/calories.dartlib/src/onehz/workout/workout_detect.darttest/onehz/calories_vo2max_test.dart
usedFitnessModel reported whether a usable VO2max arrived, not whether the regression that reads it evaluated. A bout spent entirely under the gate, or a day entirely under the flex point, is Harris-Benedict or Mifflin from end to end and has no fitness term in it, so the flag was crediting a calculation that never ran. The detector dropped the flag on the floor the way it used to drop usedDefaultAnchors, so a bout came back with no way to tell which of the two models had priced it. ExerciseSession carries it now. The gap cap is a named constant rather than a default-argument literal. A caller scoring a bout live has to stop standing in for a missing sample at the same point the re-score does, and a default argument cannot be referenced from outside, so the second copy of 150.0 was guaranteed to drift.
…iring A minute can clear the flex gate and still contribute nothing: at a low VO2max the regression comes out negative, clamps to zero and loses to the basal minute. Setting the flag on entering the branch reported a fitness-priced day whose active total was 0.0, which is the same over-claim one level down from the one the last change removed. The detector's copy of the flag had no test at all — hardcoding it to false left the suite green, which is how the flag it replaces came to be dropped on the floor in the first place. Both are pinned now, through detect() rather than a hand-built session. The detector also passed its own split threshold as the sample cap. Same number, different decision, and restating one as the other means moving the cap rescores auto-detected bouts against a value nothing else uses.
the only VO2max this app can produce is the Uth estimate, 15.3 * HRmax/HRrest with a Tanaka HRmax. substituted into keytel's fitness-adjusted regression it collapses to (1285.7 - 4.3268*age)/RHR — no VO2 term survives, just age and resting HR wearing a fitness label. the estimate's error is wider than the spread of the thing it estimates, so using it at full weight moves the number without adding information, and it made a trait metric jitter by tens of kcal a day on ordinary overnight resting-HR drift. so the fit* coefficients, the vo2max parameters, the plausibility bounds and the usedFitnessModel flags all come back out. calorie output is unchanged from before the model landed. what stays is Calories.defaultMergeGapCapS. the cap is not only this package's business — edge's live scorer has to stop standing in for a missing HR sample at the same instant the re-score does, and a default-argument literal cannot be read from another package, so the second copy of 150.0 was going to drift. the detector's note about it was also wrong: the cap CAN bind inside a detected bout, because the bridge window is twice the merge gap and _bridgeRuns stitches across an HR-free dropout of up to 300 s. a bout straddling a ~252 s dropout comes out 324.62 kcal capped against 349.44 uncapped.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/src/onehz/workout/workout_detect.dart`:
- Around line 417-427: Add a regression test alongside the existing workout
detector tests that feeds WorkoutDetector.detect a bout containing a bridged
dropout longer than Calories.defaultMergeGapCapS but still within bridgeGapS.
Assert the detected session’s calorie estimate is capped at 150 seconds,
verifying the mergeGapCapS: Calories.defaultMergeGapCapS contract rather than
testing Calories.estimateBoutCalories directly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: acd7616d-2f19-4c65-b11e-0763f042608b
📒 Files selected for processing (2)
lib/src/onehz/workout/calories.dartlib/src/onehz/workout/workout_detect.dart
Every other case for the gap cap calls estimateBoutCalories directly, which cannot observe whether the detector hands it the right constant. A dropout of 252 s is longer than the cap and shorter than the bridge window, so it lands inside one bout and the cap binds — about 24 kcal on this fixture.
abdulsaheel
changed the title
abdulsaheel
added a commit
to OpenStrap/edge
that referenced
this pull request
Aug 11, 2026
… re-score (#227) * Calories: stop double-counting basal, make the live gauge survive its re-score Calories are computed in four places and three of them had drifted apart. All four now route through one shared implementation. kAlgoVersion 61 -> 62: every fix below moves persisted numbers, so finalized days recompute. 1. THE DAY DOUBLE-COUNTS THE BASAL MINUTE. `_keytelCaloriesWake`, a derivation-local copy of Keytel, summed the FULL active rate per active minute, while `calories_total` came from `Calories.dailyEnergy`, whose active component nets out the basal minute already inside the total. The same minute was paid for twice: `calories` ran high by basalPerMin x active-minutes (~70 kcal on a day with one hard hour, scaling with active time), and because the Health export writes BASAL_ENERGY_BURNED as `calories_total - calories`, basal ran low by the same amount. The local copy is deleted; both figures come from one `wakeDayEnergy` pass, so `total - active == basal` holds by construction. 2. THE LIVE GAUGE DID NOT SURVIVE ITS OWN RE-SCORE. The tick billed the raw Keytel active rate for every second the band reported a heart rate, with no activity gate and no resting floor, while `estimateBoutCalories` bills the Harris-Benedict resting rate below RHR + 0.30*HRR. The two differ by ~2x at 70 bpm, so warm-up, inter-set rest and cool-down were charged at roughly double and the session re-scored lower than the number the athlete watched all hour. The tick now recomputes from the retained per-minute series through the same rates and gate — recompute, not accrue, matching what accrueHr already does for strain, which is what lets a resting HR landing mid-session re-score the whole bout instead of only the seconds after it arrived. 3. THE FITNESS ANCHOR WAS COMPUTED AND IGNORED. Keytel publishes a second, more accurate model that reads VO2max, and `vo2maxEstimate` was already being computed for the Body screen. The consumer that benefits most never saw it, so everyone was scored against the cohort's mean fitness. Threaded through all four paths via one shared `vo2maxFor` helper. No usable resting HR means no fitness term and the original model, never a fabricated fitness level. No crossday dependency: Uth VO2max is a function of age and resting HR, so each site resolves it locally and day derivation stays deterministic. Requires OpenStrap/analytics#42 (Calories gains an optional vo2max) merged and the pinned SHA bumped; this does not compile against the current pin. Tests: three new files, expected values hand-computed from the published equations so they fail if the paths ever agree on a wrong number. The parity test calls the real re-score rather than a stand-in — a stand-in kept passing the old model when the fitness term went in, which is exactly how these drifted. workout_calorie_anchors_test loses one case that is no longer reachable: with a resting floor a costed session is always positive. * calories: one pass per day, and a live gauge that bills real seconds follow-up on the same defect class the previous commit was aimed at. the calorie work had four things still wrong with it. 1. the day still ran TWO dailyEnergy calls. _applyWakeDayFeatures wrote calories and calories_total from wakeDayEnergy, then _stepsAndEnergy ran its own dailyEnergy further down the same day block and overwrote calories_total with it — gated on profile.isComplete instead of hasCalorieAnchors, over a flat 1440 minutes instead of the covered span, and with no vo2max. so the persisted pair came from two different estimates and health_export's BASAL_ENERGY_BURNED = total - active was off by -21/+45/+138 kcal/day at rhr 45/55/80. the duplicate is deleted; the TDEE block is published once, from the one pass, next to the scalars it belongs to. 2. the day's active calories had quietly moved from the wake series to the whole-day series, so sleep was billed as exercise. dailyEnergy's flex gate is 0.50*HRmax = 104 - 0.35*age, i.e. 79.5 bpm at 70, which an ordinary sleeping heart rate clears — 2,640 kcal a night of invented active energy for a 70y at 82 bpm. active is the wake span again, and the basal floor stays pro-rated over the whole covered day because basal metabolism does not stop overnight. said plainly in wakeDayEnergy's doc and in the version note, and onehz_pipeline is marked as the mirror rather than a second definition. 3. the live tick scored per minute-mean. that put the activity gate on the minute where estimateBoutCalories puts it on the sample, so a sawtooth sitting at the gate billed 1.19 kcal/min against the re-score's 3.24 — about 123 kcal over a zone-2 hour. it also billed a completed minute a flat 60 s whatever backed it, billed the minute in progress its SAMPLE COUNT as seconds, and lost a contact-loss gap the re-score charges. it now keeps seconds-at-each-bpm and bills per sample, gap cap included, which is what the re-score does. new parity cases for each of those; the old ones all used a 1 Hz stream transitioning exactly on a minute boundary and could not see any of it. 4. the vo2max anchor and the sex normalisation each existed twice with different mappings. both now live in profile.dart as vo2maxAnchor and workoutSex, on raw values so the isolate-side pipeline can use them too. that also fixes TRIMP, which tested sex == 'f' while the calorie path beside it accepted 'female' — a profile written by the profile screen got female calories and male strain off one field. also: the live activity pushed a literal 0 kcal to the lock screen for a session it correctly refused to score, while the in-app gauge showed "—". strain and calories are nullable through the channel now and the widget renders the absence. kAlgoVersion stays 62; its note is rewritten to describe what the code actually does now, including the wake-vs-whole-day split and which of these move a persisted day. still needs analytics#42 merged and the pin bumped before this compiles. * pin the gap cap itself, not just gap handling The contact-loss case uses a 61 s outage, which bills the same whether the 150 s bound is there or not, so it proved the two sides agree about gaps without proving either of them stops. A 200 s outage separates them: 389 billed seconds against 439 of wall clock. * gate the fitness anchor on a night, and share the gap cap The two calorie paths were reading different resting heart rates. The pipeline takes the sleep-gated one and the engine took the day's published rhr scalar, which with no sleep session is nocturnalRhr over WAKING heart rate. That is not a resting HR: it reads high, Uth divides by it, and the fitness model then prices the day as though the user were deconditioned. The pipeline already refuses that value for the same reason, so the engine does too, and a day without a night falls back to the published age/mass/sex model rather than guessing at a fitness level. The live gauge restated the 150 s gap cap as its own literal. It reads the analytics constant now, since the whole point of that path is to stop standing in for a missing sample at the instant the re-score does. The rounded triple in the day block is asserted within a kcal. The three ints are rounded independently off one double triple, and round(a + b) is not round(a) + round(b) once the fractions carry, so exact equality was passing on the arithmetic of one fixture. The exact form is still asserted on the doubles, where it actually holds. * say what the gate actually does, and mirror the pipeline properly The v62 note still described the fitness model as running whenever the day carries a resting HR, and promised that days without one are unaffected. Both are now false: the predicate is a SLEEP-DERIVED resting HR, and a no-sleep day that does carry an rhr scalar moves. Describing a change as the opposite of what it does is worse than not describing it. The gate tested the window alone. A window can be non-null while its slice comes back empty, and the pipeline then falls back to whole-day HR — so a day could still land on two different Keytel models, one step further in. It tests for substrate inside the window now, which is what the pipeline means. The no-sleep test compared a slept day against a no-sleep one, but dropping the window also stops the wake series excluding the sleep hours, so it grew from four hours to six and two of those sit above the flex point for that profile. The number moved for a reason unrelated to the anchor, and the assertion passed with the gate removed. Both comparisons hold the series fixed now and vary only the anchor. * drop the fitness model, and stop imputing a height into the day's calories The VO2max anchor is out. Uth over Tanaka collapses to (1285.7 - 4.3268*age) divided by resting HR, so there is no VO2 in it — it is resting heart rate wearing a fitness label, and its error is wider than the spread of the thing it claims to estimate. Feeding it to Keytel's fitness-adjusted model moved a trait metric by tens of kcal a day on ordinary night-to-night resting-HR noise. The sleep gate went with it: that gate existed only to decide whether the anchor was trustworthy, so with no anchor `wakeDayEnergy` has no business taking a resting HR at all. The day's calorie triple now requires a real height and goes absent without one. `dailyEnergy` defines active as the surplus over the Mifflin basal minute, so the height term sits inside the active figure as well as the total — the 170 cm stand-in was moving both. On a 35 y / 80 kg male with 600 wake minutes at 130 bpm, 150 cm against 195 cm is active 6500 vs 6383 and total 8068 vs 8232. Those are persisted to day_result and exported to Apple Health and Health Connect, so the stand-in was writing a body the user does not have into their health record — larger than the double-count this change removes. Publishing active alone for a height-less profile is not available: recovering it would mean not netting the basal minute out, which is the double-count. The 1 Hz pipeline's early-read calories gates the same way, so Today does not show a figure the derived day then withdraws. What the change is actually for is unchanged. The day's energy came from two implementations that disagreed: a derivation-local Keytel sum billed the full rate on every active minute while a second dailyEnergy call, differently gated and over a flat 1440 minutes, overwrote calories_total on the way out. Both scalars and the TDEE block now come from one pass, so total - active == basal holds by construction and the basal the Health export derives by subtraction is the figure the pass produced. calories_basal is carried for that block alone and is deliberately not a day_result scalar. The live gauge still bills a per-bpm seconds histogram through the same per-sample gate, resting floor and gap cap as the substrate re-score, and workoutSex is still the one sex normalisation — including the TRIMP constant that read 'f' alone and scored a profile stored as 'female' as male. The v62 note said the session paths are scored on read and carry no algo version of their own. Only the second half is true: stopWorkout writes the live figure into sessions.calories and the re-score only replaces it when the band handed over 90% of the window, so a sparse session keeps the live number permanently. Corrected, along with the rest of the note. * pin analytics at the named gap cap The live workout scorer reads Calories.defaultMergeGapCapS so it stops standing in for a missing heart-rate sample at the same instant the re-score of the same stream does. That constant only exists from this commit onward. --------- Co-authored-by: abdulsaheel <abdulsaheel@users.noreply.github.com>
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
2 participants
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.
estimateBoutCaloriescaps how long one HR sample may stand in for when thenext one is late, at 150 s. It was a default-argument literal, which cannot be
referenced from another package — so edge's live workout scorer restated the
same 150.0 and the two were guaranteed to drift apart the day the cap moved.
A live gauge has to stop standing in for a missing sample at the instant the
re-score of the same stream does, or the two disagree by whatever a dropout ran
over.
WorkoutDetectorwas passing its ownmergeGapSas the cap. Same number today,different quantity, and the cap really does bind inside a detected bout:
bridgeGapSis twicemergeGapS, so an HR-free dropout of up to 300 s isstitched into one bout. On a 252 s dropout that is 324.62 kcal against 349.44.
This PR originally added Keytel's second published active-energy model, the one
with a VO2max term. That is dropped. The VO2max reaching it was not measured but
estimated from the resting-HR ratio, and substituting that estimator into the
regression reduces to age and resting HR with no oxygen measurement anywhere in
it — an anchor whose error is wider than the spread of the quantity it estimates,
applied at full weight, turning a trait that moves over months into something
that moved with last night's sleep. The shared constant is what is left.