Fix sleep stage classification: movement integration, outlier filtering#19
Merged
Fix sleep stage classification: movement integration, outlier filtering#19
Conversation
…lier tightening Root cause: the rule-based classifier over-classified REM (67.7% on left side) because the default fallback in classifyEpoch returned .rem instead of .light, and REM detection had no movement or HRV gating. Changes to SleepAnalyzer: - New analyze() signature accepts movement records + calibration quality - Rewritten classifyEpoch decision tree with priority-ordered rules: calibration check > movement-based wake > deep (hrRatio<0.92) > REM (requires hrv<25 AND movement<30) > default Light - Movement-only mode when calibrationQuality < 0.3 - Null HRV never defaults to REM - Temporal smoothing post-pass removes isolated single-epoch noise - Transition constraints enforce Light between Wake<>Deep, Deep<>REM - Windowed median filter rejects HR readings >2 SD from 5-min rolling median - Tighter outlier thresholds: HR 45-130, BR 8-25 for sleep context - Quality score capped at 50 when calibration is poor Changes to HealthScreen: - fetchVitals passes movement records and piezo calibration quality - smoothedVitals filter aligned to same sleep-context thresholds Tests updated with new cases for default-Light, REM gating, deep threshold, calibration cap, movement-only mode, and BR outlier filtering. Fixes #18 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe SleepAnalyzer sleep stage classification algorithm has been significantly enhanced to accept and integrate movement data and calibration quality parameters. The Changes
Sequence DiagramsequenceDiagram
actor HS as HealthScreen
participant SA as SleepAnalyzer
participant FO as filterOutliers
participant BM as buildMovementLookup
participant CE as classifyEpoch
participant TS as applyTemporalSmoothing
participant TC as applyTransitionConstraints
participant QS as computeQualityScore
HS->>SA: analyze(vitals, movement, calibrationQuality)
activate SA
SA->>FO: filterOutliers(vitals)
activate FO
FO->>FO: Apply sleep thresholds (HR 45-130, BR 8-25)
FO->>FO: 5-min windowed HR median filter
FO-->>SA: filtered vitals
deactivate FO
SA->>BM: buildMovementLookup(movement)
activate BM
BM-->>SA: movement by minute map
deactivate BM
SA->>CE: classifyEpoch per epoch
activate CE
alt calibrationQuality < 0.3
CE->>CE: Movement-first logic
else
CE->>CE: Consider HR, HRV, BR, movement
end
CE-->>SA: sleep stages
deactivate CE
SA->>TS: applyTemporalSmoothing(epochs)
activate TS
TS-->>SA: smoothed epochs
deactivate TS
SA->>TC: applyTransitionConstraints(epochs)
activate TC
TC-->>SA: constrained epochs
deactivate TC
SA->>QS: computeQualityScore(calibrationQuality)
activate QS
QS->>QS: Cap score if calibrationQuality < 0.3
QS-->>SA: quality score
deactivate QS
SA-->>HS: SleepAnalysisResult
deactivate SA
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 6
🧹 Nitpick comments (2)
Sleepypod/Views/Data/HealthScreen.swift (1)
194-205: Extract one shared sleep-context filter.
smoothedVitalsmirrors the hard cutoffs but not the rolling-median rejection inSleepypod/Services/SleepAnalyzer.swift:124-183, so charts can still show spikes that the analyzer drops before staging. If the goal is aligned filtering, this should come from one shared helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sleepypod/Views/Data/HealthScreen.swift` around lines 194 - 205, smoothedVitals duplicates the "sleep-context" cutoffs but omits the rolling-median rejection implemented in SleepAnalyzer (lines ~124-183); extract that logic into a shared helper (e.g., a function like isValidSleepContext(v: VitalsRecord) or filterVitalsSleepContext) placed in a common module/utility and replace the inline filter in the smoothedVitals computed property with a call to that helper, and update SleepAnalyzer to use the same helper so both components apply identical cutoffs plus the rolling-median rejection.SleepypodTests/SleepAnalyzerTests.swift (1)
129-134: Give the fixtures explicit timestamps.
makeVitalstamps every record withDate()at construction time, so the new rolling-window filter, smoothing, and transition logic are all exercised against essentially the same moment. Passing a deterministic timestamp/index into the helper will make these cases much better at catching temporal regressions.Suggested refactor
- private func makeVital(hr: Double, hrv: Double, br: Double = 14) -> VitalsRecord { + private func makeVital(hr: Double, hrv: Double, br: Double = 14, timestamp: Int) -> VitalsRecord { // Use custom init via JSON decode let json = """ - {"id":\(Int.random(in: 1...99999)),"side":"left","timestamp":\(Int(Date().timeIntervalSince1970)),"heartRate":\(hr),"hrv":\(hrv),"breathingRate":\(br)} + {"id":\(Int.random(in: 1...99999)),"side":"left","timestamp":\(timestamp),"heartRate":\(hr),"hrv":\(hrv),"breathingRate":\(br)} """ return try! JSONDecoder().decode(VitalsRecord.self, from: json.data(using: .utf8)!) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SleepypodTests/SleepAnalyzerTests.swift` around lines 129 - 134, The helper makeVital currently embeds Date() into the JSON, producing non-deterministic timestamps that foil temporal tests; change makeVital's signature (makeVital(hr: Double, hrv: Double, br: Double = 14, timestamp: Int)) or similar to accept an explicit timestamp (or Date converted to Int) and use that value when constructing the JSON used to decode VitalsRecord, then update test callers to pass deterministic timestamps/indices so the rolling-window, smoothing and transition logic are exercised with stable, reproducible times.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sleepypod/Services/SleepAnalyzer.swift`:
- Around line 226-229: The
classifyEpoch(hr:hrv:br:avgHR:movement:calibrationQuality:) function currently
receives br but never uses it; either remove the br parameter or, per the
requirement, incorporate breathing rate into the REM vs deep/light decision
path. Update classifyEpoch to safely unwrap br (it's Optional Double), apply the
BR thresholds used elsewhere for outlier filtering, and use those thresholds to
bias classifications toward REM when BR is in the REM range (and away from deep
when BR is low/high as defined by the spec). Modify the REM/deep/light branch in
classifyEpoch to consult br alongside hr/hrv/movement (e.g., when movement is
low and br matches REM range prefer .rem over .deep/.light), and ensure unit
tests or callers still pass an optional br or adjust call sites if you remove
the parameter.
- Around line 255-263: The high-HR fallback currently returns .rem for hrRatio >
1.10 && mov < 30 without verifying HRV; update the condition in SleepAnalyzer
(the branch using hrRatio, mov and hrv) so that .rem is only returned when
hrRatio > 1.10, mov < 30 and hrv is present and below the REM threshold (hrv <
25); otherwise fall through (or return .wake/.unknown as appropriate) to enforce
the “positive REM evidence required” rule. Ensure you reference and use the
existing variables hrRatio, mov and hrv and the .rem/.wake enum cases when
adjusting the condition.
- Around line 283-299: The predicate in applyTemporalSmoothing currently mutates
epochs while reading neighbors, which rewrites any A-B-C pattern; change it to
take an immutable snapshot (e.g., let snapshot = epochs) and iterate using
snapshot for comparisons, and only replace epochs[i] when snapshot[i-1].stage ==
snapshot[i+1].stage && snapshot[i].stage != snapshot[i-1].stage so that only
true single-epoch spikes are flattened; keep creating the new SleepEpoch with
the original center epoch's start/duration/heartRate/hrv/breathingRate but
assign the stage from the matching neighbors.
- Around line 233-237: Calibration branch uses a higher movement threshold for
wake (mov > 300) than the normal path (mov > 200); change the degraded-mode wake
cutoff to match the normal path. In the block that checks calibrationQuality
(where variables calibrationQuality and mov are used), replace the mov > 300
wake condition with mov > 200 so movement-only logic is as sensitive as the
normal path, leaving the mov > 100 light threshold and final light return
unchanged.
- Around line 119-129: The filterOutliers function currently only filters HRV >
300 but misses non-positive HRV; update the hard-limit filter in filterOutliers
(working on VitalsRecord entries) to reject hrv values that are <= 0 as well as
> 300 so the physiological constraint "HRV > 0 and <= 300" is enforced; locate
the hrv check in filterOutliers and change it to return false when hrv <= 0 or
hrv > 300.
In `@Sleepypod/Views/Data/HealthScreen.swift`:
- Around line 347-353: The code defaults a failed api.getCalibrationStatus call
to 1.0 which can bypass SleepAnalyzer's movement-only safeguard; instead, on
error reuse the last known calibration score or fail closed (e.g., treat as 0.0)
so unverified vitals aren't trusted. Modify the block that sets
calibrationQuality (the let calibrationQuality and the api.getCalibrationStatus
call using metricsManager.selectedSide) to fetch and use a stored
lastKnownCalibrationScore (or set to 0.0 if no history) and update that stored
value whenever a successful status.piezo?.qualityScore is returned; ensure
SleepAnalyzer (movement-only threshold ~0.3) receives this safer value.
---
Nitpick comments:
In `@Sleepypod/Views/Data/HealthScreen.swift`:
- Around line 194-205: smoothedVitals duplicates the "sleep-context" cutoffs but
omits the rolling-median rejection implemented in SleepAnalyzer (lines
~124-183); extract that logic into a shared helper (e.g., a function like
isValidSleepContext(v: VitalsRecord) or filterVitalsSleepContext) placed in a
common module/utility and replace the inline filter in the smoothedVitals
computed property with a call to that helper, and update SleepAnalyzer to use
the same helper so both components apply identical cutoffs plus the
rolling-median rejection.
In `@SleepypodTests/SleepAnalyzerTests.swift`:
- Around line 129-134: The helper makeVital currently embeds Date() into the
JSON, producing non-deterministic timestamps that foil temporal tests; change
makeVital's signature (makeVital(hr: Double, hrv: Double, br: Double = 14,
timestamp: Int)) or similar to accept an explicit timestamp (or Date converted
to Int) and use that value when constructing the JSON used to decode
VitalsRecord, then update test callers to pass deterministic timestamps/indices
so the rolling-window, smoothing and transition logic are exercised with stable,
reproducible times.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24f22a04-3356-4111-a168-a3d90c13b36e
📒 Files selected for processing (3)
Sleepypod/Services/SleepAnalyzer.swiftSleepypod/Views/Data/HealthScreen.swiftSleepypodTests/SleepAnalyzerTests.swift
Comment on lines
+226
to
+229
| private func classifyEpoch( | ||
| hr: Double, hrv: Double?, br: Double?, avgHR: Double, | ||
| movement: Int?, calibrationQuality: Double | ||
| ) -> SleepStage { |
There was a problem hiding this comment.
br is threaded through but never used.
classifyEpoch accepts breathing rate, but nothing in this decision tree reads it. So the PR currently filters BR outliers without implementing the linked requirement that BR should help separate REM from deep/light.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sleepypod/Services/SleepAnalyzer.swift` around lines 226 - 229, The
classifyEpoch(hr:hrv:br:avgHR:movement:calibrationQuality:) function currently
receives br but never uses it; either remove the br parameter or, per the
requirement, incorporate breathing rate into the REM vs deep/light decision
path. Update classifyEpoch to safely unwrap br (it's Optional Double), apply the
BR thresholds used elsewhere for outlier filtering, and use those thresholds to
bias classifications toward REM when BR is in the REM range (and away from deep
when BR is low/high as defined by the spec). Modify the REM/deep/light branch in
classifyEpoch to consult br alongside hr/hrv/movement (e.g., when movement is
low and br matches REM range prefer .rem over .deep/.light), and ensure unit
tests or callers still pass an optional br or adjust call sites if you remove
the parameter.
Comment on lines
+347
to
+353
| // Fetch calibration quality for sleep analysis | ||
| let calibrationQuality: Double | ||
| if let status = try? await api.getCalibrationStatus(side: metricsManager.selectedSide) { | ||
| calibrationQuality = status.piezo?.qualityScore ?? 0.0 | ||
| } else { | ||
| calibrationQuality = 1.0 // Assume good calibration if status unavailable | ||
| } |
There was a problem hiding this comment.
Don’t treat an unknown calibration score as 1.0.
Sleepypod/Services/SleepAnalyzer.swift:233-238 only enters movement-only mode below 0.3. Defaulting a failed getCalibrationStatus call to 1.0 disables that safeguard and analyzes unverified vitals as fully trusted. Reusing the last known score or failing closed would be safer.
Suggested fix
- } else {
- calibrationQuality = 1.0 // Assume good calibration if status unavailable
+ } else {
+ calibrationQuality = 0.0 // Unknown quality; avoid bypassing degraded mode
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sleepypod/Views/Data/HealthScreen.swift` around lines 347 - 353, The code
defaults a failed api.getCalibrationStatus call to 1.0 which can bypass
SleepAnalyzer's movement-only safeguard; instead, on error reuse the last known
calibration score or fail closed (e.g., treat as 0.0) so unverified vitals
aren't trusted. Modify the block that sets calibrationQuality (the let
calibrationQuality and the api.getCalibrationStatus call using
metricsManager.selectedSide) to fetch and use a stored lastKnownCalibrationScore
(or set to 0.0 if no history) and update that stored value whenever a successful
status.piezo?.qualityScore is returned; ensure SleepAnalyzer (movement-only
threshold ~0.3) receives this safer value.
- Require HRV < 25 for high-HR REM path (was returning .rem without HRV check) - Reject non-positive HRV in outlier filter (hrv <= 0 || hrv > 300) - Align degraded-mode wake threshold to 200 (was 300, inconsistent with normal path) - Fix temporal smoothing: use immutable snapshot, only smooth A-B-A spikes (not A-B-C transitions) - Fail closed on calibration fetch error (0.0, not 1.0) - Document BR parameter as intentionally unused until core provides real BR data Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
3 tasks
6 tasks
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.
Summary
Fixes #18.
Problem
The rule-based classifier produced wildly wrong stage distributions: left side showed 67.7% REM (should be ~25%) because the fallthrough at line 133 defaulted to
.remfor any HR ratio between 0.95-1.1x average. Right side showed 30.8% Wake from noisy HR spikes (piezo quality was 0.0).Changes
SleepAnalyzer.swiftHealthScreen.swiftSleepAnalyzerTests.swiftKey decisions
Test plan
defaultIsLight,remRequiresEvidence,deepSleepThreshold,lowCalibrationCapsScore,lowCalibrationMovementOnly,brOutlierFiltering🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements