Fix/numerical correctness - #46
Merged
Merged
Conversation
a nan was taking down the whole crossday bundle. readiness_glassbox emitted percentileOfYou = nan under 7 days of history, round6 passed it through, and over in edge jsonEncode threw and the catch dropped everything — illness, anomaly, ctl/atl/tsb, chronotype, sleep coach, vo2max, every percentile. every user's first week, and forever for anyone whose resp_rate or skin_temp_z is sparse. round6 returns double? now and Metric.toJson treats a non-finite scalar as absent, so there's one chokepoint instead of a landmine per call site. lombScargle was returning variance-normalised power that everything downstream read as ms². real total power of 1260 ms² was rendering as 0.1. it returns physical psd now, and the band powers are welch-averaged over segments sized to resolve each band instead of a grid 19x too coarse — lf_hf was shipping at half its converged value. that also made it faster, 1191ms to 637ms a night. cpc is withdrawn. cpc_ratio divided by the rr periodogram hf/lf came out at 1.0000085, because the "respiration surrogate" was the rr series itself. it was never cardiopulmonary coupling. rest of it: - dropped runs in rr_correction never advanced the clock, so 299s of real time came out as 294s and inflated cvhr on exactly the noisy nights - auto_detect only closed a span on a low-hr dip, so an off-wrist gap bridged two 12-min efforts 40 min apart into one 64-min workout - cardio_stager bridged 10 min of wake where webster/cole, cited by name three lines up, allow 4 - forced windows, mad=0 and empty staging abstain now instead of reporting 0h slept, 0% - illness cusum and the anomaly detector window on calendar days, not rows numbers that moved: lf/hf/total/lf_hf, ulf is absent rather than 0.0, and on a real night wake went 17.5 to 26.0 min with tst 516.5 to 508.0. edge needs a kAlgoVersion bump.
…seline second validation round: the skeptics for analytics all died on a usage limit the first time, so these findings had never been reviewed. one skeptic per module tried to refute each, and anything it couldn't settle went to a second reviewer told to reproduce it instead. 5 claims died there. the sleep family read AccelSample.x/y/z and never .valid, so a second with no gravity — which edge hands over as exact (0,0,0) — scored as perfectly still. zAngle(0,0,0) is 0.0 and 0.0 never changes, so eight hours of missing accelerometer was the stillest possible night. it abstains now, with a reason. segmentSleep filled wall-clock seconds the record had no sample for with the label 'wake' and then counted them as measured WASO and into the efficiency denominator — absence billed as an observation, in the direction that makes your sleep look worse. the ewma baseline engine, with no history, invented a personal baseline at the midpoint of the metric's physiological BOUNDS and then published z, delta, ratio and in_normal_range against it. a number computed against a made-up anchor is worse than no number. percentileOfYou had no idea which direction was better and always called a high percentile good — including for resting heart rate, where it is not. recommendedWake added sleep minutes to a bedtime that was built as an IN-BED time, so the wake target was systematically early by the awake fraction. rsaRespRate searched 0.15-0.40 Hz only, so any real breathing rate above 24 br/min came back as a confidently wrong lower number instead of absent. cusumChangePoints centred on the median/MAD of the whole series including the post-change data, self-normalising away the step it was there to detect. dailyActiveMinutes treated list-adjacent minutes as consecutive, but the series only emits minutes that had a sample — so a coverage gap didn't break a bout. workout_detect.dart is deleted with its tests — the retroactive WorkoutDetector had no caller outside its own tests, edge no longer writes the detected_workouts stub it would have fed, and auto-detected bouts reach the ui through workout_suggestions instead. i restored it once on the strength of an edge comment saying a pass was going to be re-homed there, then found the same round had already rewritten that comment to say the opposite. the two repos were coordinated; i wasn't.
temp_circadian's nonparametric block seeded m10, l5 and their onsets from the grand mean before checking whether the hour-of-day profile was complete, so a charging window at 03:00 produced "warmest and coolest both start at midnight, amplitude 0" inside a present metric. nullable and omitted now. left the sibling in sleep/circadian_np alone — edge only ever feeds it days that passed the complete-hourly-profile admission. menstrualCoverline's threshold compared a value in one unit against a default in another and the mismatch was narrated in a comment instead of fixed. the coverline moves. chronotype returned a coverage note for what is actually an index-alignment precondition, so the caller was told to wear the band more when the two lists it was handed were built on different grids. vo2maxEstimate and physiologicalAge took a required sex and never read it. forgivingStreak and Streak were public and barrel-exported in a product that bans streaks. Readiness.meaningful was computed and serialized with no reader. the Sri class sat under an SRI header in a file with no SRI in it. all out, with their exports. two doc claims that had drifted off their code: the "spearman-style rank correlation" is a pearson, and the "default 9.21" is dimension-aware.
onehz/device.dart: a DeviceFamily enum, deviceFamilyOf() that returns null for anything it doesn't recognise, and calibrationFor() that returns null rather than handing back gen4's constants. no registry, no plugin table, no shared constants file — a metric declares its own map next to itself and refuses when the family isn't in it. unknown is a refusal, not a default, because the whole point is that a number calibrated for one strap is not a number for another. journal correlations ran 9 numeric fields against 4 outcomes behind a per-test gate. that's 36 simultaneous tests, so roughly two spurious "meaningful" findings per user were guaranteed by construction. benjamini-hochberg over the grid now, and the empty result is a real answer the card has to be able to say. habits are custom fields with max==1, so a 0/1 variable was getting a spearman rho and a theil-sen "slope per unit" — a group difference wearing the wrong clothes. they route to difference-of-means now, which was already written. hr recovery publishes tau: a fitted constant off a 180s tail with a residual gate, alongside hrr-60 rather than replacing it. it abstains often and that's correct — the recovery is biphasic and a single exponential conflates the phases, so it's conditioned and gated instead of always answering. weekday effect is kruskal-wallis first, then a permutation test on the max deviation, so the 7-way selection is paid for. without that it's a machine for manufacturing superstitions about saturdays. sleep runs terminate at unobserved instead of merging across it. the apnea screen returns the per-cycle depths and widths it was already accumulating and throwing away. vo2maxEstimate and physiologicalAge are gone — 15.3·maxHr/rhr is k/rhr, a restatement of a line already on screen, and the age function then counted rhr twice in the same direction.
sleep stages come out as ranges now, and the interval is derived from the night's own segmentation confidence rather than a published kappa. quoting one literature figure uniformly is itself a fabricated precision, and it's the kind that gets read back to you. deep carries a heavier caveat than rem. exact values stay available for investigate. the apnea screen gets its 30-night personal distribution, weighted by each night's analyzed hours, and it excludes nights the rhythm screen flagged — af produces the same cyclic rr pattern, so those nights were the screen agreeing with itself. never a per-night value, never a severity band, and the note still says what it is not. the temperature rhythm is the first metric onto the device seam, and it's the clearest case for one: gen4 counts and gen5 centi-degrees were about to share an array with nothing to tell them apart. each family carries its own constants next to the metric. m10, l5 and ra are withheld rather than nulled — ra's denominator crosses zero on a median-centred series, so the honest move is not to publish the ratio at all. the observed heart-rate ceiling is per-family too, because a max held on a chest strap and one held on wrist ppg are not the same measurement. it needs fifteen continuous seconds with corroborating motion, or one artifact raises the ceiling forever and drags every zone boundary with it. zones anchor on the 28-day median resting hr instead of a single night. cycle day counts from every logged start, not just the last one, so a cycle from march stops counting past its own length. cycle lengths sit next to the published criterion with no verdict attached. the change-point detector's penalty comes off full-series variance, so it under-splits by construction — that's in the note, because absence of a change-point is not evidence of stability. sri hands back the day pairs it was already computing. the last universal 220-age fallback in calories is gone.
hrvFreq built its lomb-scargle time axis by cumulatively summing rr intervals, so every sensor dropout was spliced out and the night was reconstructed as continuous. measured across 13 real nights, the beat clock covered 0.71-0.87 of the true wall span on gen4 and as little as 0.13 on MG. it now re-anchors to the real rec_ts at a dropout and every night lands within 1% of wall. lf/hf moves, and it moves across the line people read as sympathetic vs vagal: MG 08-12 0.65 to 1.53, gen4 08-13 1.32 to 2.03, whoop5 08-11 0.72 to 1.25. vlf is where the splicing hurt most — MG 08-13 goes 4,153 to 136,046 ms2. the guard is deliberately LOCAL — this beat's wall step against this beat's own interval — not accumulated drift. whole-second rr_ts_ms means every in-run wall step is 0 or 1000ms against an 860ms interval, so a drift-based guard would re-anchor on nearly every beat and throw away the only sub-second information that exists. it also means the gen4-vs-gen5 clock disagreement doesn't get resolved by the back door: it's recorded at the site, with the reason it isn't decidable, and no scale factor invented to paper over it. three other findings fall out of the same fix. rsa respiration stops publishing 9.00 br/min on a night it should abstain on. the apnea screen's six wall-clock constants are finally measured on a wall clock. the 30-minute hrv bins are actually 30 minutes instead of 32-36. a cold strap read as more ready — the skin-temp input had no warm-up or off-wrist gate and is oriented goodSign -1, so a two-hour cold start displaced one night by 32.9 adc counts, larger than the whole daily rhythm, worth about +26 points. there's a settled-fraction now and the input goes absent for a night that can't be settled. the circadian day-run was cleared by a trailing incomplete day, and today is always incomplete, so IS/IV and the cosinor were absent every day forever while telling the user they had no data about a corpus with 88-97% coverage. and two equations that didn't match their papers: two of eight three-process constants (one off by 10.9x) and a BIC penalty at half its own value.
StepParams.gain was 1.11, documented as calibrating "raw 90 -> ~100 ground truth". measured on clean synthetic gait the raw counter is already exact — raw/truth 1.00 at 60, 80, 100, 120, 140 and 180 spm — so the multiplier was manufacturing a +10.6% over-count, and it has live callers, so it has been shipping. the 90->100 figure is only reproducible at about 0.050 g peak amplitude, sitting on a cliff where the count goes 0 -> 56 -> 98 -> 100 across 0.048 to 0.055 g. fitted at the detector's unstable knee, n=1. the real still->walk->still deficit is a FLAT -0.9% from a 20-step bout to a 10,000-step bout. flat is the tell: a gain error scales and an offset shrinks, so it is neither — it's per-minute chunk-boundary loss inside calcSteps. left alone deliberately and documented as not-a-gain, so nobody re-derives 1.11 from the symptom. added AN-2554's two step-interval bounds, which our port never had, against the buffer's real sample rate rather than a hardcoded 100. the ceiling fires exactly at the physiological line: 300 spm still counts, 305 doesn't. 250 and 300 are INSIDE 5 steps/s and rejecting them would need a bound tighter than the note states, so they stand. the floor is dead at 100 hz (maxMinTimeout already reads 0 below ~32 spm) and live below it — at 50 hz, 25 spm went from 250 to 0. 40 spm still counts and should; 0.67 steps/s is a slow walk, not an artifact. inside 60-180 spm nothing moved, to the step. and the citation was wrong, which is the same failure this project has been bitten by twice — a wrong reference makes a thing look checked. it is not AN-2554: that runs at 50 hz on the sum of absolutes. this is its peak and dynamic-threshold structure on euclidean magnitude retuned for 100 hz, plus zhao's ADXL345 confirm mechanic, with sens and confirm values that appear in neither ancestor and were never validated by either.
ran pedometer() against OxWalk — 39 adults 19-81, axivity ax3 on the dominant wrist at 100hz, an hour each of unscripted free living, heel strikes annotated from a synchronised foot-facing camera. cc by, oxford wearables group. chunked at 6000 samples, exactly the way production chunks. MAPE 33.0%, bias -13.6%, totals -19.8%. five of thirty-nine land within ten percent. bland-altman limits are -1109 to +470 steps over ONE HOUR. and there is no gain that fixes it, which is the important part. the fitted multiplier is 1.247 by total ratio and 1.18 by minimising MAPE — but the error CHANGES SIGN with activity. the 22 participants who actually walked under-count by a uniform -25.4%; the 17 who barely moved average +1.7% around a 42.9% MAPE because two of them over-count by 3x (217 true -> 650, 258 -> 685). applying 1.18 buys five points of a thirty-three point error and pushes those two to +254% and +214%. it is 1.11 in a better suit. gain stays 1.00. the interval guards do nothing here — 33.0 on, 33.4 off — because the 320 spm case they exist for isn't in this corpus. kept: free, and correct at the bound. 25hz is dead. native 25hz wrist is 91.9% MAPE, -87.5% on totals, and NINE participants read exactly zero. decimating locates the cliff between 50 and 33hz. the cause is that every constant except the interval bounds is in SAMPLES — window=33 is 1.32s at 25hz, wider than a gait cycle. the live 0x33 frame rate is documented nowhere in this tree, so that is now a shipping gate. two things i did not expect. the hip has almost the same MAPE (29.5) and a completely different SHAPE: its worst over-count across 39 free-living hours is +5.3%, the wrist's is +199.5%, twice. at the hip the error is one-sided and bounded, at the wrist two-sided and unbounded above. P28 is +166% at the wrist and exactly 0 at the hip. and production's per-minute chunking is a RESCUE, not the -0.9% cost the docstring calls it. the sweep bottoms out exactly at 6000 and degrades monotonically to -55.1% for one contiguous hour, with 8 of 39 counting zero over the full hour: dynVal locks high on one large non-gait event and never recovers, and the minute reset re-seeds it. the cross-boundary state-carrying fix the docstring proposes would make real signal substantially worse. do not implement it. a naive vm peak detector gets 137.9% MAPE at the wrist against our 33.0, so confirm=8 is load-bearing and the algorithm is not the problem. the wrist is.
resp rate was withheld on 17 of 40 nights with "HF peak unstable across spectral resolutions". neither story i expected was true: the spreads aren't clustered just above the 3.0 threshold and they aren't bimodal, they're a smooth continuum straddling it with no gap — four nights sit within 1 br/min of the line on either side. pass or fail was a coin flip. and coverage didn't explain it either: the withheld nights have median 1371 worn minutes and 349 minutes of sleep. full nights. the cause is that lomb-scargle is an INCONSISTENT estimator — its variance doesn't fall as the record lengthens. over an 8h night the true bin spacing is 3.5e-5 hz while the 300-point grid steps 0.0012 hz, so the three grids were sampling bins about thirty apart. refining the grid never picked between two close peaks; it re-rolled the dice. there were ~2600 local maxima in the hf band every night, top-to-second power ratio 0.77 to 0.99. and breathing drifts 19 to 16 br/min across a night anyway, so a single whole-night spectrum smears a peak that is genuinely moving. so the published values were wrong too, not just the absences. 08-14 published 10.66 br/min where that night's own sub-windows agree on 16.97. one night published 9.30, which is 0.3 above the 0.15 hz floor — a floor-pinned pick. the same night replayed with a 24-minute-longer window was withheld once and published 17.56 the other time. welch segmentation now: 300s sub-windows at 50% overlap, each with its own nyquist ceiling, the rate is the median across them, and the gate is half the sub-windows agreeing within 2 br/min. agreeBrpm wasn't raised, it's gone. the threshold is calibrated against a surrogate null — his own nights with the NN values shuffled, which destroys RSA and keeps the sampling geometry. surrogate consensus 15-28%, real 52-85%, chance 19%. 50% sits in the measured gap instead of being a number someone liked. 14 of 15 replayable nights publish, was 9. the spread tightens from 9.3-19.4 to 16.4-18.4 br/min. where the old estimator already agreed with itself the value moves under 0.35; the four big moves are exactly the nights it had spiked. the header no longer claims pimentel 2017. that citation described varying AR model order and this never did. one cost, stated: the all-day 3-minute curve gets stricter, 83% to 65% present, because a 3-min window now has to agree across three 90s sub-windows. values unchanged where both publish, and it's ~10x cheaper per window.
|
Warning Review limit reached
Next review available in: 42 minutes Limit details: You’ve used the included review currently available. 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 within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (81)
📝 WalkthroughWalkthroughThis change updates the one-hertz analytics package across clinical, human, motion, respiration, sleep, wellness, and workout modules. It adds new metrics, removes deprecated APIs, adds device-family calibration, preserves timestamp gaps, and expands regression and dataset validation coverage. ChangesClinical, foundations, and statistical utilities
Human analytics and coaching
Motion and activity
Respiration and recovery
Sleep
Wellness and device calibration
Workout analytics
Validation and tooling
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🔴 Critical · up to This PR changes numerical behavior across several production metrics and validation paths, but the current head does not pass analysis and still has issues that can publish incorrect results, reject valid data, throw exceptions, or hang on certain inputs. It is not merge-ready until the compile failure and the major correctness/runtime issues are fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant SensorData
participant DeviceDispatch
participant OneHzMetric
participant MetricJson
SensorData->>DeviceDispatch: provide device family and sensor samples
DeviceDispatch->>OneHzMetric: select family calibration or return refusal
OneHzMetric->>OneHzMetric: apply gap-aware analysis and abstention rules
OneHzMetric->>MetricJson: serialize metric values and notes
MetricJson-->>SensorData: return JSON-compatible result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
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 |
only 3.5.0 flagged them, stable didn't. tests deliberately call the deprecated surface — that's the point of them.
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 by CodeRabbit
New Features
Improvements
Removals