A sedentary day is not eight naps: keep sedentary wake in the awake baseline - #39
Merged
Conversation
…aseline #38's awake HR baseline excluded the main sleep and EVERY detected bout. Since every stretch of >=5 min of wrist immobility is a bout, that removed all of the day's still time, and what was left was the AMBULATORY HR median wearing the name "awake baseline". The dip gate `medHr > baseline * napRestingHrMult` is then cleared by any motionless awake stretch whose HR sits more than 5% below walking HR: desk work, reading, driving, a sofa. Measured on the merged code, not argued. A synthetic day of 8 x (6 min walking @ 96 bpm, 25 min motionless @ 72 bpm) — nobody napped — returned EIGHT naps totalling 199 minutes, each at confidence 0.85, the cap. An unremarkable 10% contrast (84 vs 76 bpm) did the same. Keeping the still seconds puts the median at 72, and 0.95 x 72 = 68.4 < 72 rejects all eight. The change was reaching for two real exclusions, and both are kept: * The CANDIDATE's own low-HR seconds, or a bout is graded against a median it is itself dragging down. That is inherently per-candidate, so the baseline now moves inside the loop: the day's awake pool minus THIS bout. * Any UNFINISHED bout. The nap window deliberately runs hours past midnight, so the first hours of tonight's sleep sit in the record; that is sleep, not sedentary wake, and belongs in no baseline. Still excluded, day-wide. What is no longer excluded is ordinary daytime stillness — which is precisely what an awake baseline is supposed to be made of. Absence stays honest. Moving the baseline per-candidate moved the "not enough awake HR" abstain with it, and a day whose only still block cannot be judged must not come back as an empty list that every caller reads as "judged, none". A new `noBaseline` counter carries that case, and when nothing was emitted and at least one candidate went unjudged the day returns Metric.absent with the count — restoring at day level what the whole-day check used to give for free. The existing "too few awake HR samples" test pins it. Tests: three new cases, and they are not decorative. Reintroducing the bug (drop the `if (!unfinished[b]) continue;` guard) fails exactly these three, 26 -> 23, while EVERY pre-existing nap test still passes — the old suite structurally could not catch this, because its false-positive fixture uses bpm 80 for both its active and its still segments, so active and sedentary HR are identical and the baseline cannot be inflated. The contrast is the bug, so it is now in the fixture. The third test is the other direction: a genuine 56 bpm nap on an otherwise sedentary day is still detected, so this does not over-correct into missing real naps. dart analyze clean; 409 tests green.
📝 WalkthroughWalkthroughNap detection now retains sedentary wake heart rates, computes a separate baseline for each candidate bout, and reports absent metrics when candidates cannot be judged. Tests cover sedentary false positives and genuine low-heart-rate naps. ChangesNap detection
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 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/sleep/nap.dart`:
- Around line 332-357: Add a regression fixture in the nap tests that gives
detectNaps an awakeIdx pool meeting minAwakeHrSamples but leaves fewer than that
threshold after removing the candidate interval. Assert that detectNaps returns
Metric.absent when no other nap is detected, ensuring the per-candidate baseline
shortage branch is exercised.
In `@test/onehz/nap_test.dart`:
- Around line 425-450: Update the test “a real nap is still detected on a
SEDENTARY day” to insert an active interval longer than the bout-chain bridge
immediately before the 50-minute nap, separating it from the surrounding
25-minute sedentary bouts. Keep the existing single-nap assertion and
additionally assert that the detected nap’s tstSec is approximately 50 * 60.
🪄 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: 276617ac-8a85-46c2-804b-085c53c637d8
📒 Files selected for processing (2)
lib/src/onehz/sleep/nap.darttest/onehz/nap_test.dart
Comment on lines
+332
to
+357
|
|
||
| // PER-CANDIDATE baseline: the day's awake pool minus THIS bout. A bout must | ||
| // not be graded against a median it is itself pulling down — see the pool | ||
| // construction above for why only the candidate and the deferred bouts come | ||
| // out, and not every still block in the day. | ||
| final awakeHr = <double>[]; | ||
| for (final k in awakeIdx) { | ||
| if (k >= start && k < end) continue; | ||
| awakeHr.add(hr[k]); | ||
| } | ||
| if (awakeHr.length < minAwakeHrSamples) { | ||
| // The day had enough awake HR, but not once this candidate is removed — | ||
| // so THIS bout cannot be corroborated, while others still may be. Abstain | ||
| // for it rather than judging it against a median built from a handful of | ||
| // seconds. Counted separately from `unverifiable` because it is the one | ||
| // rejection that makes the DAY's verdict incomplete: see the check after | ||
| // the loop. | ||
| noBaseline++; | ||
| continue; | ||
| } | ||
| final baseline = median(awakeHr)!; | ||
| if (baseline <= 0) { | ||
| noBaseline++; | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add a regression test for the per-candidate baseline shortage path.
test/onehz/nap_test.dart Lines 322-336 leave the day-wide awakeIdx pool below minAwakeHrSamples. That test returns at Line 268. It does not execute the new check at Line 342.
Add a fixture where awakeIdx.length >= minAwakeHrSamples, but removing the candidate leaves fewer samples. Assert that detectNaps returns Metric.absent when no other nap is emitted.
🤖 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/nap.dart` around lines 332 - 357, Add a regression
fixture in the nap tests that gives detectNaps an awakeIdx pool meeting
minAwakeHrSamples but leaves fewer than that threshold after removing the
candidate interval. Assert that detectNaps returns Metric.absent when no other
nap is detected, ensuring the per-candidate baseline shortage branch is
exercised.
Comment on lines
+425
to
+450
| test('a real nap is still detected on a SEDENTARY day', () { | ||
| // The other side of the same coin, and the reason the fix keeps sedentary | ||
| // wake in the pool instead of dropping every bout: a day that is mostly | ||
| // sitting still must still be able to report the one block that was | ||
| // actually sleep. Baseline lands at desk HR (~72), and 56 clears | ||
| // 0.95 x 72 = 68.4 comfortably. | ||
| final d = _Day(); | ||
| for (var b = 0; b < 4; b++) { | ||
| d | ||
| ..active(6, bpm: 96) | ||
| ..still(25, bpm: 72); | ||
| } | ||
| d.still(50, bpm: 56); // the genuine nap, a real autonomic dip | ||
| for (var b = 0; b < 4; b++) { | ||
| d | ||
| ..active(6, bpm: 96) | ||
| ..still(25, bpm: 72); | ||
| } | ||
| d.active(60, bpm: 96); // end awake: nothing to defer at the record end | ||
|
|
||
| final m = detectNaps(d.accel, d.hr); | ||
|
|
||
| expect(m.value, hasLength(1), | ||
| reason: 'the deep-dip block is a nap even though the day around it ' | ||
| 'is sedentary'); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Separate the 50-minute nap from adjacent sedentary bouts.
The preceding still(25) and following still(25) are contiguous with still(50). The detector therefore evaluates one approximately 100-minute immobility bout. hasLength(1) passes even if it reports the combined bout instead of the intended 50-minute nap.
Insert an active interval longer than the bout-chain bridge before the nap. Assert that tstSec is close to 50 * 60.
Proposed test correction
for (var b = 0; b < 4; b++) {
d
..active(6, bpm: 96)
..still(25, bpm: 72);
}
+ d.active(6, bpm: 96);
d.still(50, bpm: 56); // the genuine nap, a real autonomic dip
for (var b = 0; b < 4; b++) {
d
..active(6, bpm: 96)
..still(25, bpm: 72);
}
@@
expect(m.value, hasLength(1),
reason: 'the deep-dip block is a nap even though the day around it '
'is sedentary');
+ expect(m.value!.single.tstSec, closeTo(50 * 60, 90));🤖 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/nap_test.dart` around lines 425 - 450, Update the test “a real nap
is still detected on a SEDENTARY day” to insert an active interval longer than
the bout-chain bridge immediately before the 50-minute nap, separating it from
the surrounding 25-minute sedentary bouts. Keep the existing single-nap
assertion and additionally assert that the detected nap’s tstSec is
approximately 50 * 60.
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
1 participant
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.
Follow-up to #38, which is now on
main(c3a30be).The defect
The awake HR baseline excludes the main sleep and every detected bout:
Every stretch of ≥5 min of wrist immobility is a bout, so this removes all of the day's still time. What survives is the ambulatory HR median wearing the name "awake baseline". The gate
medHr > baseline * napRestingHrMultis then cleared by any motionless awake stretch whose HR sits more than 5% below walking HR — desk work, reading, driving, a sofa.Measured, not argued
Run against the merged code on
main. A synthetic day of 8 × (6 min walking @ 96 bpm, 25 min motionless @ 72 bpm) — nobody napped:An unremarkable 10% contrast (84 vs 76 bpm) does the same, at 0.84. Keeping the still seconds puts the median at 72, and
0.95 × 72 = 68.4 < 72rejects all eight — the exclusion is what breaks it.Nothing consumes this yet (edge is pinned to
f0d1153, pre-nap), so no user is affected today. edge#204 exists to consume it.What this keeps
#38 was reaching for two real exclusions, and both survive:
What is no longer excluded is ordinary daytime stillness, which is precisely what an awake baseline is supposed to be made of. #38's own regression test (
a real nap is still detected on a sleep-dominated window) still passes.Absence stays honest
Moving the baseline per-candidate moved the "not enough awake HR" abstain with it. A day whose only still block cannot be judged must not return an empty list, which every caller reads as judged, none. A new
noBaselinecounter carries that case: when nothing was emitted and at least one candidate went unjudged, the day returnsMetric.absentwith the count. The existingtoo few awake HR samples abstains rather than setting a thresholdtest pins this — it failed on the first cut of this change, which is how the gap was found.The tests are not decorative
Reintroducing the bug (drop the
if (!unfinished[b]) continue;guard) fails exactly the three new tests, 26 → 23, while every pre-existing nap test still passes.That is the point: the old suite structurally could not catch this. Its false-positive fixture (
still but with no HR dip is desk work, not a nap) usesbpm: 80for both its active and its still segments — active and sedentary HR are identical, so the baseline cannot be inflated. The contrast is the bug, so the contrast is now in the fixture.The third new test runs the other way: a genuine 56 bpm nap on an otherwise sedentary day is still detected, so this does not over-correct into missing real naps.
Verification
dart analyzecleandart test→ 409 passingNote for edge#204
This changes no public signature —
detectNaps,NapWindow,tstSec/tibSec/efficiencyare all untouched. If it lands before #204 repins, #204 should pin to this merge commit rather thanc3a30be.Summary by CodeRabbit