fix: IS divisor uses populated hour-bins instead of epochsPerDay - #67
Conversation
phase-locked missing data (charging at the same hour every day) leaves a hour-of-day bin permanently unsampled, so profile.length < epochsPerDay and the old profVar/profile.length divisor inflated interdailyStability by epochsPerDay/profile.length. matches circadian_np.dart's p=epochsPerDay.
There was a problem hiding this comment.
Sorry @abdulsaheel, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 6 days and 8 hours by commenting @sourcery-ai review. Upgrade to get a review now.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe interdaily-stability calculation now divides profile variance by ChangesCircadian stability calculation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix 🚥 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 |
Reviewer's GuideFixes nonparametric interdaily stability normalization by dividing profile variance by fixed Flow diagram for fixed epochs-per-day IS normalizationflowchart TD
A["Hourly samples across analysis window"] --> B["Build hour-of-day profile"]
B --> C["One hour bin is permanently unpopulated"]
C --> D["Compute profile variance"]
D --> E["Divide by fixed epochsPerDay"]
E --> F["Calculate interdaily stability"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
…not raw span (#68) same bug shape as the IS fix (#67). a contact-loss dropout (arm tucked, band shifted) turns part of the ratio-of-ratios NaN, but odiPerHour and burdenPct still divided by the full wall-clock span, diluting the rate instead of reporting it over the time that actually had usable samples. now scales the denominator by trustedCoverage, same idea cvhr_apnea.dart already uses.
found this while going through the circadian stuff. _nonparam's IS calc divides profVar by profile.length (count of hour-of-day bins that have at least one sample) instead of the fixed epochsPerDay. circadian_np.dart already does this right with p = epochsPerDay.
the bug only shows up when some hour-of-day is never sampled across the whole window, e.g. charging at the same hour every day (this file's own header calls that scenario out). that hour gets dropped from profile, profile.length < epochsPerDay, and IS gets inflated by epochsPerDay/profile.length. when every hour has a sample it's a no-op since profile.length == epochsPerDay, which is why it's been hiding.
added a test with 14 days of hourly data and hour 3 permanently missing. confirmed it fails on the old code (1.0 clamped) and passes on the fix (0.9583, matches the predicted 24/23 inflation factor).
Summary by Sourcery
Fix nonparametric interdaily stability normalization for sampling windows with missing hour-of-day bins.
Bug Fixes:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests