fix: abstain instead of fabricating across the metric families - #30
Conversation
An audit of the whole package found a recurring shape: when an input was
absent or a dispersion estimate was degenerate, the code substituted a
constant instead of returning an absent Metric. Every fix below restores
the package's core contract — absent input yields null, never a number —
and each is covered by a regression test that fails against the old code.
The two worst were in sleep, where a night with no data reported as a
perfect night:
- advanced_stager: a window with ZERO accelerometer samples returned one
full-length 'light' segment, so a strap on the nightstand produced
tst=28800 at 100% efficiency. The unbounded accel carry-forward did
the same for a 6 h dropout. Windows are now split at long gaps, the
carry-forward is bounded to 60 s, and unusable seconds stay wake. The
carry-forward also copied the stale timestamp, mis-centring every
RMSSD/LF-HF window inside a gap.
- cardio_stager: with HR entirely absent the baseline fell back to a
literal 60 bpm, after which every HR-relative gate was dead and every
epoch defaulted to NREM. It now abstains.
- Webster rescoring read the list it was mutating, so bridged wake runs
inflated the context for the next one and cascaded; a fragmented night
came back with waso=0 and 100% efficiency. Context is now snapshotted
(same bug in stager.dart).
Degenerate-dispersion fallbacks that fired instead of abstaining:
- anomaly: a constant quantized column was clamped to a 1e-6 scale, so a
0.4 change became a ~1e6 z and flagged an illness anomaly every time.
- illness_cusum: a max(1.0, SD) floor latched a sustained red from a
5 bpm one-night bump on a quantized baseline.
- stress_si: a 1 ms RR range divided through to SI 48780 ('high').
- cpc: a zero or NaN-poisoned spectrum published a 999.0 sentinel, and a
non-finite band power sailed past the variance guard entirely.
Method and unit errors:
- rr_correction: the Lipponen-Tarvainen threshold was taken over |dRR|
rather than the signed series, collapsing the quartile deviation ~15x
into a fixed 100 ms cutoff. An artifact-free 400-beat record had 32
beats flagged and spline-replaced. Verified against the real capture.
- circadian_lifestyle: social jetlag medianed raw clock hours with no
wrap, so a 23:50 weekday vs 01:10 weekend midsleep reported 22.55 h.
Now circular throughout, with chronotype's bands on the same axis.
- load_trimp: two disagreeing Banister implementations (neither with the
published female scale), a sample duration taken from only the first
two timestamps (8.08 vs 46.85 strain on the same data), an unclamped
strain reaching 107.8, and CTL/ATL seeded from a single day.
- nocturnal: the "lowest 30-min mean" never slid, silently becoming the
mean of everything, and compacted across off-skin gaps.
- hrv_freq: HF was withheld but still summed into total.
- van_hees: the final win-1 seconds shared one verdict; now per-second,
with an explicit undecidable tail rather than a guess.
- resp_rate: brpm came from the median grid while peak_hz came from the
highest-power grid, so peak_hz*60 disagreed with brpm.
Honesty envelope:
- physiologicalAge returned a present Metric with zero physiological
inputs while claiming six in inputs_used.
- workout detection let a missing HRmax DISABLE the zone-2 gate, then
computed strain against a hidden 220-age anchor next to hrmax: null,
and derived resting HR without filtering off-skin samples.
- vo2maxEstimate emitted Infinity for restingHr == 0.
- readiness_composite disclosed "robust-z" even when the mean/SD
fallback produced the value. The fallback itself is unchanged.
Also: gapAwareEwma extrapolated on non-positive dt, prsa threw RangeError
for l=1, cosinor scored confidence on unadjusted R-squared, and cycles
truncated pre-onset beats into minute 0.
Outputs change. Consumers must bump their algorithm version to recompute.
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR tightens edge-case handling across clinical, foundation, human, respiration, sleep, wellness, and workout analytics. It adds configurable validation, abstention paths, consistent serialization, corrected time-series calculations, and regression tests covering invalid, sparse, degenerate, and irregular inputs. ChangesClinical metrics and validation
Sleep, wellness, and workouts
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Accelerometer
participant AdvancedSleepStager
participant CardioStager
participant SleepSegmentation
Accelerometer->>AdvancedSleepStager: provide samples and gaps
AdvancedSleepStager->>CardioStager: stage contiguous usable runs
CardioStager->>AdvancedSleepStager: return epochs or abstention
AdvancedSleepStager->>SleepSegmentation: return per-second stage segments
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 6
🧹 Nitpick comments (6)
lib/src/onehz/sleep/van_hees.dart (2)
160-173: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffPer-second forward scan is O(n·win) on still records.
The early
breakonly helps where movement exists; a genuinely still 24 h record does the full 300-sample scan for every second (~2.6·10⁷ comparisons). A sliding max (monotonic deque overdAng) gives the same result in O(n) if this ever runs on full-day captures.🤖 Prompt for 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. In `@lib/src/onehz/sleep/van_hees.dart` around lines 160 - 173, Replace the nested forward scan in the immobility computation with an O(n) sliding-window maximum using a monotonic deque over dAng, preserving the existing win boundary, angleThresholdDeg comparison, and fullWindow handling for immobile and immobileUnknown.
226-230: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the getter instead of re-counting.
This duplicates
SleepWindow.unresolvedTailSec's body; the count could be taken from the constructed window (or a small shared helper) to keep one definition.🤖 Prompt for 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. In `@lib/src/onehz/sleep/van_hees.dart` around lines 226 - 230, Update the unresolved count calculation near the immobileUnknown loop to reuse the constructed SleepWindow's unresolvedTailSec getter, or extract and reuse a shared counting helper, instead of manually recounting the boolean values. Keep the resulting unresolved value consistent with SleepWindow.unresolvedTailSec and avoid duplicating its logic.lib/src/onehz/sleep/cardio_stager.dart (1)
637-711: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSnapshot-based context is applied consistently (onset/lastSleep,
runBefore/runAfter, and the scan loop all readsnap), so bridging can no longer cascade. One note: this rule is now duplicated near-verbatim inlib/src/onehz/sleep/stager.dart(_websterRescore), differing only in the rule table — worth extracting a shared helper parameterized by the table at some point.🤖 Prompt for 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. In `@lib/src/onehz/sleep/cardio_stager.dart` around lines 637 - 711, The snapshot-based implementation is correct; no immediate change is required. As a follow-up, extract the duplicated _websterRescore logic shared by cardio_stager.dart and stager.dart into a helper parameterized by the rule table, while preserving each caller’s existing rules and snapshot-based behavior.lib/src/onehz/clinical/load_trimp.dart (1)
197-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
@Deprecatedso the alias is actually surfaced.
banisterScaleis documented as deprecated but carries no annotation, so no call site gets a warning and the male-only constant keeps looking like the canonical one.♻️ Proposed change
/// Deprecated alias for [banisterScaleMen]; kept so existing call sites keep /// resolving. Prefer [banisterY], which pairs c and b correctly by sex. + `@Deprecated`('Use banisterY(x, female: ...) — c and b must stay paired by sex') static const double banisterScale = banisterScaleMen;🤖 Prompt for 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. In `@lib/src/onehz/clinical/load_trimp.dart` around lines 197 - 199, Add a Dart `@Deprecated` annotation to the static const banisterScale alias, targeting the existing banisterY replacement while preserving its value and compatibility behavior. Keep banisterScaleMen and banisterY unchanged.test/onehz/workout_test.dart (1)
435-463: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant
day(...)calls — compute once and destructure.
day(workS: 400, workBpm: 160, restBpm: 60)is invoked six separate times (once per field) to build a singleWorkoutDetector.detectcall. It's currently harmless sinceday()is deterministic, but it's wasted work and a latent trap if the helper ever gains any non-deterministic behavior (the six calls could silently produce mismatched arrays).♻️ Proposed fix
final scenarios = <List<ExerciseSession>>[ - for (final anchor in <double?>[null, 190]) - WorkoutDetector.detect( - hrTs: day(workS: 400, workBpm: 160, restBpm: 60).hrTs, - hrBpm: day(workS: 400, workBpm: 160, restBpm: 60).hrBpm, - gravTs: day(workS: 400, workBpm: 160, restBpm: 60).gTs, - gx: day(workS: 400, workBpm: 160, restBpm: 60).gx, - gy: day(workS: 400, workBpm: 160, restBpm: 60).gy, - gz: day(workS: 400, workBpm: 160, restBpm: 60).gz, - restingHR: 60, - maxHR: anchor, - ), + for (final anchor in <double?>[null, 190]) + () { + final d = day(workS: 400, workBpm: 160, restBpm: 60); + return WorkoutDetector.detect( + hrTs: d.hrTs, + hrBpm: d.hrBpm, + gravTs: d.gTs, + gx: d.gx, + gy: d.gy, + gz: d.gz, + restingHR: 60, + maxHR: anchor, + ); + }(), ];🤖 Prompt for 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. In `@test/onehz/workout_test.dart` around lines 435 - 463, In the test "an emitted session NEVER reports strain without an HRmax anchor", compute the day(...) fixture once per scenario and reuse its returned timestamp and sensor arrays for the WorkoutDetector.detect call. Replace the six repeated day(...) invocations while preserving the existing null and anchored maxHR scenarios and assertions.lib/src/onehz/workout/workout_detect.dart (1)
403-422: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
calUsedDefaultAnchorsis dead in this path.WorkoutDetector.detect()only reachesestimateBoutCalories()after both anchors are non-null, so this flag can never becometruehere. If this is just future-proofing, add a short note; otherwise remove the extra pass-through.🤖 Prompt for 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. In `@lib/src/onehz/workout/workout_detect.dart` around lines 403 - 422, The calUsedDefaultAnchors assignment in WorkoutDetector.detect is dead because estimateBoutCalories is only called with non-null anchors; remove this unused local and its pass-through assignment, while preserving the existing kcal and kj calculations.
🤖 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/clinical/load_trimp.dart`:
- Around line 495-505: Update ctlAtlTsb around the minDays validation and prime
seed calculation so nonpositive minDays cannot allow an empty dailyTrimp seed.
Clamp the effective minimum-day requirement to at least one (or otherwise guard
the empty sublist), preserve the existing Metric.absent abstention contract, and
ensure mean(dailyTrimp.sublist(0, prime)) receives at least one value.
In `@lib/src/onehz/clinical/nocturnal.dart`:
- Around line 60-75: In the nocturnal heart-rate averaging logic, update both
eligibility checks around the initial `best` calculation and the rolling mean
calculation to require `count > 0` before dividing. Preserve the existing
`needValid` threshold while preventing fully off-skin windows from producing or
publishing NaN through `sum / count`.
In `@lib/src/onehz/clinical/stress_si.dart`:
- Around line 133-137: Update the guard in the stress index calculation after
mxdmnMs is computed to unconditionally reject a zero range, while retaining the
configurable minRangeMs threshold and modeS validation. Ensure constant segments
return null even when minRangeMs is 0, preventing the subsequent AMo calculation
from dividing by zero.
In `@lib/src/onehz/human/coaching.dart`:
- Around line 339-360: Filter physiological inputs through an isFinite
validation before adding them to used, and apply the same finite-value guard in
each downstream scoring block so NaN or infinity cannot affect score, physioAge,
or deltaYears. Update the relevant PhysioAge calculation logic around the used
list and per-input consumers while preserving the absent result when no finite
physiological inputs are available.
- Around line 544-551: Update the meaningful-effect calculation around bigEnough
and separated so a defined, sufficiently large Cohen’s d can qualify the result
even when pct is null for zero-centred outcomes. Preserve the existing
percentage threshold for non-null pct values, and use the existing
minCohensD/separated logic without changing the constant-outcome handling.
In `@lib/src/onehz/sleep/stager.dart`:
- Around line 218-222: The test seam documentation incorrectly claims
websterRescoreAutonomic is not publicly exported, while the test accesses it
through the onehz.dart barrel; apply the same correction to
websterRescoreCardio. Either change the tests to import the relevant src files
directly and keep both symbols out of the barrel, or remove the “not part of the
public barrel” claims and document both functions as supported public entry
points.
---
Nitpick comments:
In `@lib/src/onehz/clinical/load_trimp.dart`:
- Around line 197-199: Add a Dart `@Deprecated` annotation to the static const
banisterScale alias, targeting the existing banisterY replacement while
preserving its value and compatibility behavior. Keep banisterScaleMen and
banisterY unchanged.
In `@lib/src/onehz/sleep/cardio_stager.dart`:
- Around line 637-711: The snapshot-based implementation is correct; no
immediate change is required. As a follow-up, extract the duplicated
_websterRescore logic shared by cardio_stager.dart and stager.dart into a helper
parameterized by the rule table, while preserving each caller’s existing rules
and snapshot-based behavior.
In `@lib/src/onehz/sleep/van_hees.dart`:
- Around line 160-173: Replace the nested forward scan in the immobility
computation with an O(n) sliding-window maximum using a monotonic deque over
dAng, preserving the existing win boundary, angleThresholdDeg comparison, and
fullWindow handling for immobile and immobileUnknown.
- Around line 226-230: Update the unresolved count calculation near the
immobileUnknown loop to reuse the constructed SleepWindow's unresolvedTailSec
getter, or extract and reuse a shared counting helper, instead of manually
recounting the boolean values. Keep the resulting unresolved value consistent
with SleepWindow.unresolvedTailSec and avoid duplicating its logic.
In `@lib/src/onehz/workout/workout_detect.dart`:
- Around line 403-422: The calUsedDefaultAnchors assignment in
WorkoutDetector.detect is dead because estimateBoutCalories is only called with
non-null anchors; remove this unused local and its pass-through assignment,
while preserving the existing kcal and kj calculations.
In `@test/onehz/workout_test.dart`:
- Around line 435-463: In the test "an emitted session NEVER reports strain
without an HRmax anchor", compute the day(...) fixture once per scenario and
reuse its returned timestamp and sensor arrays for the WorkoutDetector.detect
call. Replace the six repeated day(...) invocations while preserving the
existing null and anchored maxHR scenarios and assertions.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ace1c92-047e-4a33-bb70-d7d0a315a1a9
📒 Files selected for processing (31)
lib/src/onehz/clinical/cosinor.dartlib/src/onehz/clinical/hrv_freq.dartlib/src/onehz/clinical/illness_cusum.dartlib/src/onehz/clinical/load_trimp.dartlib/src/onehz/clinical/nocturnal.dartlib/src/onehz/clinical/prsa.dartlib/src/onehz/clinical/readiness_lnrmssd.dartlib/src/onehz/clinical/stress_si.dartlib/src/onehz/foundations/baseline.dartlib/src/onehz/foundations/rr_correction.dartlib/src/onehz/human/circadian_lifestyle.dartlib/src/onehz/human/coaching.dartlib/src/onehz/respiration/resp_rate.dartlib/src/onehz/sleep/advanced_stager.dartlib/src/onehz/sleep/cardio_stager.dartlib/src/onehz/sleep/cpc.dartlib/src/onehz/sleep/cycles.dartlib/src/onehz/sleep/segment.dartlib/src/onehz/sleep/stager.dartlib/src/onehz/sleep/van_hees.dartlib/src/onehz/wellness/anomaly.dartlib/src/onehz/wellness/readiness_composite.dartlib/src/onehz/workout/workout_detect.darttest/onehz/clinical_test.darttest/onehz/coaching_test.darttest/onehz/foundations_test.darttest/onehz/human_test.darttest/onehz/respiration_test.darttest/onehz/sleep_honesty_test.darttest/onehz/wellness_test.darttest/onehz/workout_test.dart
| if (dailyTrimp.length < minDays) { | ||
| return Metric<LoadState>.absent( | ||
| tier: Tier.estimate, | ||
| inputs_used: inputs, | ||
| note: 'no daily TRIMP history', | ||
| note: needBaselineNote(have: dailyTrimp.length, need: minDays), | ||
| ); | ||
| } | ||
| final lc = 1 - math.exp(-1 / ctlDays); | ||
| final la = 1 - math.exp(-1 / atlDays); | ||
| var ctl = dailyTrimp.first; | ||
| var atl = dailyTrimp.first; | ||
| for (var i = 1; i < dailyTrimp.length; i++) { | ||
| final prime = math.min(math.max(primeDays, 1), dailyTrimp.length); | ||
| final seed = mean(dailyTrimp.sublist(0, prime))!; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
mean(...)! can throw when minDays is overridden to ≤ 0.
The length < minDays gate is the only thing keeping prime ≥ 1. With ctlAtlTsb([], minDays: 0) the gate passes, prime becomes 0, sublist(0, 0) is empty and mean returns null, so the ! throws. Clamping minDays (or guarding the empty list) keeps the abstain contract intact for every parameterization.
🛡️ Proposed guard
- if (dailyTrimp.length < minDays) {
+ if (dailyTrimp.isEmpty || dailyTrimp.length < minDays) {
return Metric<LoadState>.absent(
tier: Tier.estimate,
inputs_used: inputs,
- note: needBaselineNote(have: dailyTrimp.length, need: minDays),
+ note: needBaselineNote(have: dailyTrimp.length, need: math.max(1, minDays)),
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (dailyTrimp.length < minDays) { | |
| return Metric<LoadState>.absent( | |
| tier: Tier.estimate, | |
| inputs_used: inputs, | |
| note: 'no daily TRIMP history', | |
| note: needBaselineNote(have: dailyTrimp.length, need: minDays), | |
| ); | |
| } | |
| final lc = 1 - math.exp(-1 / ctlDays); | |
| final la = 1 - math.exp(-1 / atlDays); | |
| var ctl = dailyTrimp.first; | |
| var atl = dailyTrimp.first; | |
| for (var i = 1; i < dailyTrimp.length; i++) { | |
| final prime = math.min(math.max(primeDays, 1), dailyTrimp.length); | |
| final seed = mean(dailyTrimp.sublist(0, prime))!; | |
| if (dailyTrimp.isEmpty || dailyTrimp.length < minDays) { | |
| return Metric<LoadState>.absent( | |
| tier: Tier.estimate, | |
| inputs_used: inputs, | |
| note: needBaselineNote(have: dailyTrimp.length, need: math.max(1, minDays)), | |
| ); | |
| } | |
| final lc = 1 - math.exp(-1 / ctlDays); | |
| final la = 1 - math.exp(-1 / atlDays); | |
| final prime = math.min(math.max(primeDays, 1), dailyTrimp.length); | |
| final seed = mean(dailyTrimp.sublist(0, prime))!; |
🤖 Prompt for 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.
In `@lib/src/onehz/clinical/load_trimp.dart` around lines 495 - 505, Update
ctlAtlTsb around the minDays validation and prime seed calculation so
nonpositive minDays cannot allow an empty dailyTrimp seed. Clamp the effective
minimum-day requirement to at least one (or otherwise guard the empty sublist),
preserve the existing Metric.absent abstention contract, and ensure
mean(dailyTrimp.sublist(0, prime)) receives at least one value.
| double? best; | ||
| if (count >= needValid) best = sum / count; | ||
| for (var i = windowSamples; i < hr.length; i++) { | ||
| if (hr[i] > 0) { | ||
| sum += hr[i]; | ||
| count++; | ||
| } | ||
| final out = hr[i - windowSamples]; | ||
| if (out > 0) { | ||
| sum -= out; | ||
| count--; | ||
| } | ||
| if (count >= needValid) { | ||
| final m = sum / count; | ||
| if (best == null || m < best) best = m; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard count > 0 before dividing.
With a caller-supplied minCoverage <= 0, needValid is 0, so a fully off-skin window passes the eligibility test and sum / count is 0/0 → NaN, which then gets published as low30Mean (NaN comparisons make the m < best check silently accept it).
🛡️ Proposed guard
- if (count >= needValid) best = sum / count;
+ if (count > 0 && count >= needValid) best = sum / count;
@@
- if (count >= needValid) {
+ if (count > 0 && count >= needValid) {
final m = sum / count;
if (best == null || m < best) best = m;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| double? best; | |
| if (count >= needValid) best = sum / count; | |
| for (var i = windowSamples; i < hr.length; i++) { | |
| if (hr[i] > 0) { | |
| sum += hr[i]; | |
| count++; | |
| } | |
| final out = hr[i - windowSamples]; | |
| if (out > 0) { | |
| sum -= out; | |
| count--; | |
| } | |
| if (count >= needValid) { | |
| final m = sum / count; | |
| if (best == null || m < best) best = m; | |
| } | |
| double? best; | |
| if (count > 0 && count >= needValid) best = sum / count; | |
| for (var i = windowSamples; i < hr.length; i++) { | |
| if (hr[i] > 0) { | |
| sum += hr[i]; | |
| count++; | |
| } | |
| final out = hr[i - windowSamples]; | |
| if (out > 0) { | |
| sum -= out; | |
| count--; | |
| } | |
| if (count > 0 && count >= needValid) { | |
| final m = sum / count; | |
| if (best == null || m < best) best = m; | |
| } |
🤖 Prompt for 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.
In `@lib/src/onehz/clinical/nocturnal.dart` around lines 60 - 75, In the nocturnal
heart-rate averaging logic, update both eligibility checks around the initial
`best` calculation and the rolling mean calculation to require `count > 0`
before dividing. Preserve the existing `needValid` threshold while preventing
fully off-skin windows from producing or publishing NaN through `sum / count`.
| final mxdmnMs = seg.reduce(math.max) - seg.reduce(math.min); | ||
| final mxdmnS = mxdmnMs / 1000.0; | ||
| // MxDMn sits in the denominator: a range at or below the beat-timing | ||
| // resolution is not a measurement of autonomic tension, it is quantization. | ||
| if (modeS <= 0 || mxdmnMs < minRangeMs) return null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the unconditional zero-range rejection.
The old guard rejected mxdmnS <= 0 regardless of configuration. Now the only range check is mxdmnMs < minRangeMs, so a caller passing minRangeMs: 0 lets a perfectly constant segment through and amoPct / (2·modeS·0) publishes Infinity — the exact degenerate case the existing "constant RR series" test covers at the default.
🛡️ Proposed fix
- if (modeS <= 0 || mxdmnMs < minRangeMs) return null;
+ if (modeS <= 0 || mxdmnMs <= 0 || mxdmnMs < minRangeMs) return null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final mxdmnMs = seg.reduce(math.max) - seg.reduce(math.min); | |
| final mxdmnS = mxdmnMs / 1000.0; | |
| // MxDMn sits in the denominator: a range at or below the beat-timing | |
| // resolution is not a measurement of autonomic tension, it is quantization. | |
| if (modeS <= 0 || mxdmnMs < minRangeMs) return null; | |
| final mxdmnMs = seg.reduce(math.max) - seg.reduce(math.min); | |
| final mxdmnS = mxdmnMs / 1000.0; | |
| // MxDMn sits in the denominator: a range at or below the beat-timing | |
| // resolution is not a measurement of autonomic tension, it is quantization. | |
| if (modeS <= 0 || mxdmnMs <= 0 || mxdmnMs < minRangeMs) return null; |
🤖 Prompt for 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.
In `@lib/src/onehz/clinical/stress_si.dart` around lines 133 - 137, Update the
guard in the stress index calculation after mxdmnMs is computed to
unconditionally reject a zero range, while retaining the configurable minRangeMs
threshold and modeS validation. Ensure constant segments return null even when
minRangeMs is 0, preventing the subsequent AMo calculation from dividing by
zero.
| // ABSTAIN when NOTHING physiological was supplied. `score` starts at the | ||
| // chronological age and only the blocks below move it, so with every | ||
| // physiological input null this used to return a PRESENT metric reading | ||
| // "physioAge == your age, delta 0" — a fabricated result — while claiming six | ||
| // inputs it never saw. A physiological age with no physiology in it is not an | ||
| // estimate, it is the birth date restated. | ||
| final used = <String>[ | ||
| if (vo2max != null) 'vo2max', | ||
| if (restingHr != null) 'resting_hr', | ||
| if (rmssd != null) 'rmssd', | ||
| if (sleepDurationH != null) 'sleep_duration', | ||
| if (sleepEfficiency != null) 'sleep_efficiency', | ||
| if (dailySteps != null) 'steps', | ||
| ]; | ||
| if (used.isEmpty) { | ||
| return const Metric<PhysioAge>.absent( | ||
| tier: Tier.estimate, | ||
| inputs_used: ['profile'], | ||
| note: 'no physiological input present — "—" (never imputed; ' | ||
| 'chronological age alone is not a physiological age)', | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Non-finite physiology inputs still slip through the used gate.
double.nan/infinity for e.g. vo2max counts as "used", propagates into score, and yields a non-finite physioAge/deltaYears that PhysioAge.toJson emits raw and jsonEncode throws on — the exact hazard just fixed in vo2maxEstimate. Suggest filtering on isFinite here too.
🛡️ Proposed fix
- final used = <String>[
- if (vo2max != null) 'vo2max',
- if (restingHr != null) 'resting_hr',
- if (rmssd != null) 'rmssd',
- if (sleepDurationH != null) 'sleep_duration',
- if (sleepEfficiency != null) 'sleep_efficiency',
- if (dailySteps != null) 'steps',
- ];
+ bool ok(double? v) => v != null && v.isFinite;
+ final used = <String>[
+ if (ok(vo2max)) 'vo2max',
+ if (ok(restingHr)) 'resting_hr',
+ if (ok(rmssd)) 'rmssd',
+ if (ok(sleepDurationH)) 'sleep_duration',
+ if (ok(sleepEfficiency)) 'sleep_efficiency',
+ if (ok(dailySteps)) 'steps',
+ ];(the downstream blocks that consume each value need the same ok(...) condition so a NaN never reaches score)
🤖 Prompt for 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.
In `@lib/src/onehz/human/coaching.dart` around lines 339 - 360, Filter
physiological inputs through an isFinite validation before adding them to used,
and apply the same finite-value guard in each downstream scoring block so NaN or
infinity cannot affect score, physioAge, or deltaYears. Update the relevant
PhysioAge calculation logic around the used list and per-input consumers while
preserving the absent result when no finite physiological inputs are available.
| final bigEnough = pct != null && pct.abs() >= minEffectPct; | ||
| final separated = d != null | ||
| ? d.abs() >= minCohensD | ||
| // Both sides exactly constant: d is undefined. Only trust it with a | ||
| // real number of observations behind each constant. | ||
| : (delta.abs() > 0 && | ||
| tagged.length >= minNForZeroSpread && | ||
| untagged.length >= minNForZeroSpread); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Zero-centred outcomes can never be "meaningful".
bigEnough requires pct != null, but pct is null whenever |untaggedMean| < 1e-9 — true for z-scored or delta-type outcomes (skin-temp z, HRV delta). A large, well-separated effect on such a series is then always reported meaningful: false. Consider treating a defined Cohen's d as sufficient when the percentage is undefined.
- final bigEnough = pct != null && pct.abs() >= minEffectPct;
+ // A zero-centred outcome has no meaningful percentage; fall back to the
+ // standardized effect size alone rather than vetoing it outright.
+ final bigEnough = pct != null ? pct.abs() >= minEffectPct : d != null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final bigEnough = pct != null && pct.abs() >= minEffectPct; | |
| final separated = d != null | |
| ? d.abs() >= minCohensD | |
| // Both sides exactly constant: d is undefined. Only trust it with a | |
| // real number of observations behind each constant. | |
| : (delta.abs() > 0 && | |
| tagged.length >= minNForZeroSpread && | |
| untagged.length >= minNForZeroSpread); | |
| // A zero-centred outcome has no meaningful percentage; fall back to the | |
| // standardized effect size alone rather than vetoing it outright. | |
| final bigEnough = pct != null ? pct.abs() >= minEffectPct : d != null; | |
| final separated = d != null | |
| ? d.abs() >= minCohensD | |
| // Both sides exactly constant: d is undefined. Only trust it with a | |
| // real number of observations behind each constant. | |
| : (delta.abs() > 0 && | |
| tagged.length >= minNForZeroSpread && | |
| untagged.length >= minNForZeroSpread); |
🤖 Prompt for 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.
In `@lib/src/onehz/human/coaching.dart` around lines 544 - 551, Update the
meaningful-effect calculation around bigEnough and separated so a defined,
sufficiently large Cohen’s d can qualify the result even when pct is null for
zero-centred outcomes. Preserve the existing percentage threshold for non-null
pct values, and use the existing minCohensD/separated logic without changing the
constant-outcome handling.
| /// Test seam for [_websterRescore] — exposed (non-private) so the regression | ||
| /// test can drive the continuity rule directly. Not part of the public barrel. | ||
| void websterRescoreAutonomic(List<SleepStage> sm, int epochSec) => | ||
| _websterRescore(sm, epochSec); | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Doc says "not part of the public barrel", but the test reaches it through the barrel.
test/onehz/sleep_honesty_test.dart imports only package:openstrap_analytics/onehz.dart and calls websterRescoreAutonomic (line 216), so this seam is exported publicly — the comment is misleading and the symbol is now de-facto public API. Either use a src/-path import in the test and keep it out of the barrel, or drop the "not public" claim and treat it as a supported entry point. Same applies to websterRescoreCardio in lib/src/onehz/sleep/cardio_stager.dart.
🤖 Prompt for 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.
In `@lib/src/onehz/sleep/stager.dart` around lines 218 - 222, The test seam
documentation incorrectly claims websterRescoreAutonomic is not publicly
exported, while the test accesses it through the onehz.dart barrel; apply the
same correction to websterRescoreCardio. Either change the tests to import the
relevant src files directly and keep both symbols out of the barrel, or remove
the “not part of the public barrel” claims and document both functions as
supported public entry points.
Diagnosed against a real 152 MB user database: dailyStepEstimate reported 39,384 steps for a day whose true value was ~2,000. Root cause, proven by counterfactual on that raw data. ENMO is `mean(max(0, |a| - gRef))`, and gRef is auto-calibrated per day as the median of the stillest samples. On the bad day gRef came out at 0.9797; every other day it was ~1.032. The wrist rests in a different orientation during the long sleep block, and because sleep is the stillest and longest stretch it dominates that median — hourly median |a| was 0.94-0.99 asleep vs 1.03-1.06 awake, the sensor having the usual few-percent per-axis gain error. ENMO subtracts gRef from EVERY sample, so a reference 0.05 g low adds 0.05 g to every minute of the day, which is exactly the 0.05 g walking floor. Ordinary sitting cleared the gate for hours. Sweeping gRef over the identical samples: 0.97 -> 42,155 steps; 1.00 -> 13,035; 1.02 -> 0; 1.03 -> 0. There is no stable regime. The signal and the calibration error are both ~0.05 g, so SNR is ~1 by construction and no choice of threshold fixes it — the feature is wrong. Three changes, none of them a threshold tweak: FEATURE. Gravity is a constant VECTOR in the sensor frame over short windows; motion is AC. So high-pass each axis and take the magnitude of the dynamic vector, instead of estimating a scalar |g| and subtracting it. Any per-axis offset or gain error lands in the DC term and cancels exactly. Verified end to end: injecting +0.05 g of axis offset and +5% of axis gain leaves the active-minute count bit-identical. ANCHOR. Two anchors were tested and both fail. An absolute g constant (the old 0.05) is calibration-fragile, per above. A same-day relative baseline — which the old docstring falsely claimed was implemented — fails in mirror image: on a quiet day the baseline collapses, the floor collapses with it, and everything passes (it produced 16,610 on that same day). The stable anchor is a multi-day PERSONAL floor. personalDynFloor takes the pooled trailing minutes; personalDynFloorFromDailySummaries takes one persisted value per day for callers that prune their raw substrate, and uses the median across days so a single anomalous day cannot move it. With no history the estimator ABSTAINS — it does not fall back to a constant, because falling back to a constant is the bug. METRIC. Nyquist is not negotiable: gait is 1.4-2.5 Hz and 2.0 Hz (120 spm, the most common cadence) aliases exactly to DC at 1 Hz. Steps are not resolvable from this substrate; ambulatory MINUTES are, and minutes of moderate activity is the unit public activity guidance is written in. So activeMinutes becomes the primary quantity and steps are reported as a RANGE over the Tudor-Locke free-living cadence band, narrowed only when Tier A has measured this user's real cadence. The `cadence = 85 + 220*ENMO + 0.4*hrExcess` regression is deleted: it claimed to resolve cadence from a signal that provably cannot resolve cadence. Tier A (the 100 Hz AN-2554 pedometer) is untouched — it is a real, ground-truth-calibrated count and it works. Docstrings now match the implementation. The previous ones described the per-day baseline defence as present and as the reason the estimator "CANNOT inflate"; it was neither implemented nor sufficient. Outputs change for every day. Consumers must bump their algorithm version.
Added: step/activity detection rebuilt on a calibration-invariant featurePushed Root cause. ENMO is Sweeping
The signal and the calibration error are both ~0.05 g — SNR ≈ 1 by construction. No threshold value fixes this, which is why this is a feature change, not a tuning change. Three changesFeature. Gravity is a constant vector in the sensor frame over short windows; motion is AC. So high-pass each axis and take the magnitude of the dynamic vector, instead of estimating a scalar ‖g‖ and subtracting it. Any per-axis offset or gain error lands in DC and cancels exactly. There's an end-to-end property test: injecting +0.05 g of axis offset and +5% of axis gain leaves the active-minute count identical. Anchor. Two anchors were tested against the real data and both fail. An absolute g constant is the bug above. A same-day relative baseline — which the old docstring falsely claimed was implemented — fails in mirror image: on a quiet day the baseline collapses and everything passes (16,610 on that same day). The stable anchor is a multi-day personal floor. With no history the estimator abstains; falling back to a constant is the bug. Metric. Gait is 1.4–2.5 Hz and 120 spm aliases exactly to DC at 1 Hz — steps are not resolvable from this substrate. Tier A (the 100 Hz AN-2554 pedometer) is untouched — verified byte-identical modulo comments. It's a real ground-truth-calibrated count and it works. Validated on the real database: per-day summaries sit at 0.40–0.47 across ten days, and the derived floor moves only between 0.4493 and 0.4575 — absorbing a day whose own value fell to 0.3148.
|
Edge CI was failing to COMPILE, and the failure is the whole reason the v48 note existed: error • The function 'personalDynFloorFromDailySummaries' isn't defined error • The named parameter 'personalDynFloorG' isn't defined error • The getter 'activeMinutes' isn't defined for DailyStepEstimate lib/compute/ calls analytics API that only exists on the sibling PR, but pubspec.yaml still pointed at the pre-fix SHA. It compiled locally only because the gitignored pubspec_overrides.yaml redirects to ../analytics — so the green local run was green-by-omission, and CI was the only thing telling the truth. This is the same drift class as v43, caught in its loud form: a missing symbol rather than a silently different number. The v43 version of this shipped a readiness fix that the pinned SHA never contained and stayed broken for three releases. Both pins now point at the sibling PR heads — still immutable commit SHAs, never floating refs: protocol 5675b2f (OpenStrap/protocol#19) analytics 8b1aa4e (OpenStrap/analytics#30) Verified, rather than assumed, that those SHAs contain what the changelog claims: `git show 8b1aa4e:lib/src/onehz/motion/steps.dart` carries the new step API, rr_correction.dart has the signed-dRR `seg.add(x[k])`, and `git show 5675b2f:lib/src/live.dart` has kKnownRecordVersions. Also corrected the kAlgoVersion changelog. It still said the sibling fixes were NOT pinned, which stopped being true with this commit — a changelog that misdescribes what is in the build is the exact failure being guarded against, in either direction. pubspec.lock now records the real git provenance (resolved-ref) instead of the local path: entries a previous `pub get` wrote while the overrides file was present. Reproduced CI locally to confirm: with pubspec_overrides.yaml moved aside, `flutter pub get` resolves ref == resolved-ref == 8b1aa4e, `flutter analyze` is clean, and the suite is 914 passing. ON MERGE: move both pins to the resulting main commits and bump kAlgoVersion again — a PR-branch SHA can become unreachable if the branch is deleted after a squash merge.
OpenStrap/protocol#19 and OpenStrap/analytics#30 are merged, so pubspec.yaml moves off the PR-branch heads v49 pointed at and onto the resulting main commits: protocol 5675b2f -> a98cd70 analytics 8b1aa4e -> f5ccae6 A PR-branch SHA stops being the canonical location of a change once it merges, and can be orphaned outright if the branch is later deleted. kAlgoVersion 49 -> 50 even though no edge source line changed alongside it. The constant identifies the code that PRODUCED a day_result, and that code includes the pinned siblings — a device holding v49 rows built against the PR-branch SHAs must re-derive rather than serve them as equivalent. Treating "same content, different commit" as beneath a bump is the assumption that lets a stale bundle outlive a dependency change. Verified at the merge commits themselves rather than inferred from the PRs being green — the check v43 skipped: analytics f5ccae6 steps.dart has the new step API (9 symbol hits) rr_correction.dart:233 is `seg.add(x[k])` (signed dRR) advanced_stager.dart has maxAccelCarryForwardSec protocol a98cd70 live.dart has kKnownRecordVersions Reproduced CI locally with pubspec_overrides.yaml moved aside: resolved-ref matches both pins exactly, flutter analyze clean, 914 passing.
1 participant
A full-package audit found one recurring shape: when an input was absent or a dispersion estimate was degenerate, the code substituted a constant instead of returning an absent
Metric. Every fix restores the core contract — absent input yields null, never a number — and each carries a regression test that fails against the old code.The worst: a night with no data reported as a perfect night
advanced_stager— a window with zero accelerometer samples returned one full-length'light'segment. A strap left on the nightstand producedtst=28800at 100% efficiency. The unbounded accel carry-forward did the same for a 6 h dropout. Windows are now split at long gaps, the carry-forward is bounded to 60 s, and unusable seconds stay wake. (The carry-forward also copied the stale timestamp, mis-centring every RMSSD/LF-HF window inside a gap.)cardio_stager— with HR entirely absent the baseline fell back to a literal60bpm, after which every HR-relative gate was dead and every epoch defaulted to NREM. It now abstains.waso=0and 100% efficiency. Context is now snapshotted (same bug instager.dart).Measured end-to-end: nightstand night
tst 28438 → 0,efficiency 100 → 0; fragmented nightwaso 0 → 1920 s,efficiency 100.0 → 89.5.Degenerate dispersion firing instead of abstaining
anomalyclamped a constant quantized column to a1e-6scale, so a 0.4 change became a ~1e6 z and flagged an illness anomaly every time.illness_cusum'smax(1.0, SD)floor latched a sustained red from a 5 bpm one-night bump.stress_silet a 1 ms RR range divide through to SI 48780 ('high').cpcpublished a999.0sentinel, and a non-finite band power sailed past the variance guard entirely.Method and unit errors
rr_correctiontook the Lipponen–Tarvainen threshold over|dRR|rather than the signed series, collapsing the quartile deviation ~15× into a fixed 100 ms cutoff. An artifact-free 400-beat record had 32 beats flagged and spline-replaced. Verified against the realwhoop_histcapture, not just synthetic data.circadian_lifestylemedianed raw clock hours with no wrap, so a 23:50 weekday vs 01:10 weekend midsleep reported 22.55 h of social jetlag. Now circular throughout, withchronotype's bands on the same axis.load_trimpwas two unreconciled TRIMP stacks: two disagreeing Banister implementations (neither with the published female scale), a sample duration taken from only the first two timestamps (8.08 vs 46.85 strain on identical data), an unclamped strain reaching 107.8, and CTL/ATL seeded from a single day.nocturnal's "lowest 30-min mean" never slid — it was the mean of everything, compacted across off-skin gaps.hrv_freqwithheld HF but still summed it intototal.van_heesgave the finalwin-1seconds one shared verdict; now per-second with an explicit undecidable tail.resp_ratesourcedbrpmandpeak_hzfrom different grids, sopeak_hz*60disagreed withbrpm.Honesty envelope
physiologicalAgereturned a present Metric with zero physiological inputs while claiming six ininputs_used.220-ageanchor next tohrmax: null, and derived resting HR without filtering off-skin samples.vo2maxEstimateemittedInfinityforrestingHr == 0(which then throws injsonEncode).readiness_compositedisclosed "robust-z" even when the mean/SD fallback produced the value. The fallback itself is unchanged — only the disclosure string.Also:
gapAwareEwmaextrapolated on non-positive dt,prsathrewRangeErrorforl=1,cosinorscored confidence on unadjusted R², andcyclestruncated pre-onset beats into minute 0.Verification
dart test: 355 passing (was 290), 0 failing.dart analyze: clean.Outputs change. Consumers must bump their algorithm version to force recompute. The edge PR deliberately does not yet claim these fixes in its
kAlgoVersionchangelog — itspubspec.yamlstill pins the previous analytics SHA, and this must be repinned after merge, then bumped again.Summary by CodeRabbit
New Features
Bug Fixes
Tests