Delete the 1 Hz step estimate; rebuild movement minutes on measured evidence - #35
Merged
Conversation
A user reported 2,645 steps on a day they took under 400. Diagnosed on their
real DB: the number was 23 "active minutes" x an assumed 115 spm cadence. Both
halves of that conversion are invalid at 1 Hz and neither is fixable by
retuning, so the conversion is removed rather than recalibrated.
* Cadence is NOT IDENTIFIABLE. Gait is 1.4-2.3 Hz (Straczkiewicz 2023,
doi:10.1038/s41746-022-00745-z); at 1 Hz every fundamental is sub-Nyquist
and 80/100/140/160 spm alias to the same 0.333 Hz. No published step
detector exists below 10 Hz.
* The minutes were never specifically ambulation. At the wrist, arm work
out-accelerates walking (stirring ~104 mg, chopping ~139 mg vs walking
~66 mg ENMO), so a movement threshold cannot isolate gait even at full
rate: wrist devices emit 22-27 false steps/min during dishes, reaching and
driving (O'Connell 2017, doi:10.1371/journal.pone.0169616) while detecting
slow walking at sensitivity 0.05. The errors have OPPOSITE sign, so no
gain constant corrects both.
Confirmed against that DB's own ground truth: in the single window where the
100 Hz pedometer and 1 Hz data overlap, HR ran 95->108 with dynAmp 0.31-0.40 g
and the REAL count was 11 steps in 3.1 min (3.5 spm) where the estimator would
have assigned ~115 spm.
`dailyStepEstimate` -> `dailyActiveMinutes`, returning `DailyMovementEstimate`:
active minutes only, no steps, no cadence, no range. A contract test asserts
the serialised surface can never regain a step or cadence key.
Everything below was PROVEN against 4 days of real 1 Hz substrate first:
* HR GATE DELETED. `restingHr + 8 bpm` changed the answer by exactly ZERO
minutes on every day tested. At RHR ~62 it sits at ~6% of heart-rate
reserve (below every ACSM band) and 73-100% of covered minutes already
cleared it. It failed in the wrong direction too: PPG HR is least reliable
during the motion being gated, so a dropout deleted minutes the
accelerometer measured fine. The API no longer accepts HR at all.
* x3 CEILING DELETED. Rejected ZERO minutes on all 4 days with 0.42-0.55 g
of headroom, and cannot fire on artifacts (a 3 s knock averages ~0.23 g,
below the FLOOR). The only thing it could ever exclude was real exertion.
* FLOOR MUST BE FROZEN. It is derived from the signal it thresholds, so a
continuously recomputed floor cancels the trend it exists to report:
scaling a real day's dynAmp gave 37 active minutes at 1x, 1.5x, 2x AND 3x
when recomputed, versus 23 -> 254 frozen. Adds
`enrollmentDaysForFrozenFloor` and `shouldRefreezeFloor`.
REFUTED and deliberately not built: a sleep-anchored floor (CV 138.6% across
days vs 9.3% for the current estimator, and on one night it landed above the
entire day's range, which would report zero); accel autocalibration (offset
and uniform gain cancel exactly through the high-pass and the floor
normalisation -- +5% gain moves the gate decision by 0.0000; only anisotropic
gain survives at ~1-3%); gravity/forearm orientation (it solved the ambulation
problem this commit deletes).
SEMANTICS CORRECTED. The WHOOP 1 Hz accel field is a fused GRAVITY vector, not
acceleration: across 269,486 real samples ||a|| is p50 1.027 g with 0.030%
above 1.3 g, and during the single most vigorous minute of a day it measured
1.033 g +- 0.006 with 0 of 420 samples above 1.2 g. So `dynAmp` measures how
fast the wrist RE-ORIENTS, and ENMO/MAD over this substrate reduce to
~(1.03 - gRef): a pure calibration artifact with zero signal. That is the true
root cause of the 42,155-steps-at-gRef-0.97 / 0-at-1.02 collapse, and it is
now documented on the type instead of a comment claiming gravity removal
leaves "motion".
388 tests pass.
📝 WalkthroughWalkthroughThe 1 Hz Tier B motion path now reports sustained wrist-movement minutes and bouts. It no longer derives steps or consumes cadence and heart-rate inputs. Frozen personal floors, refreezing rules, and related tests were added. Changes1 Hz active-minutes measurement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MotionSamples
participant dailyActiveMinutes
participant FrozenFloor
participant DailyMovementEstimate
MotionSamples->>dailyActiveMinutes: provide covered 1 Hz motion samples
FrozenFloor->>dailyActiveMinutes: provide personal floor
dailyActiveMinutes->>dailyActiveMinutes: gate samples above the personal floor
dailyActiveMinutes->>DailyMovementEstimate: return activeMinutes and boutCount
🚥 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: 3
🤖 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/motion/steps.dart`:
- Line 724: Update the conf calculation to align the outer clamp’s upper bound
with the maximum reachable value from the inner clamp, removing the unreachable
0.45 ceiling while preserving the existing lower bound and coverage scaling.
- Around line 686-690: Update the pass-1 gate comment immediately above gateOk
to describe only the dyns[k] > floor threshold check; remove references to the
ambulatory band and HR availability, matching the simplified gate implemented in
the loop.
In `@test/onehz/steps_test.dart`:
- Around line 632-655: Update the test around dailyActiveMinutes to vary the
active block length with k, rather than always passing 60 to day, so the
frozen-floor result f genuinely increases across iterations. Preserve the
existing monotonicity assertion and recomputed-floor comparison while ensuring
the generated activity counts increase with k.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f019b2d0-ee4b-407e-8d9a-6cc565a5fa4a
📒 Files selected for processing (4)
lib/src/onehz/motion/enmo.dartlib/src/onehz/motion/motion.dartlib/src/onehz/motion/steps.darttest/onehz/steps_test.dart
…lamp
CodeRabbit findings, all three verified against the code before fixing.
* The frozen-floor test PASSED VACUOUSLY. `day(0.05*k, 0.4*k, 200, 60)` held
the active block at a fixed 60 minutes for every k, so the frozen count was
60 on every iteration and `expect(f, greaterThanOrEqualTo(lastFrozen))`
compared a constant to itself. It proved nothing while claiming to prove
the central result of the PR.
Rebuilt on a GRADED distribution, which is what real activity looks like:
activity does not scale a fixed-size block, it lifts more of a continuous
distribution above the threshold (the real-data proof moved 23 -> 254).
Frozen counts now genuinely vary (125 -> 250) and the assertion is STRICT
monotonic increase plus a 1.5x margin, so a regression to constant output
fails loudly.
* Stale pass-1 gate comment still described "movement inside the ambulatory
band" and "(when HR is available) HR lifted off rest" above a loop that
does neither. Same class of known-false comment this PR exists to remove.
* `clamp(0.30 * clamp(...), 0.1, 0.45)` — the inner term caps at 0.30, so the
0.45 outer bound was unreachable and implied a confidence this metric never
claims. Aligned to the reachable value.
Also corrected the FILE HEADER, which review did not flag but was describing
the removed design: it still promised an HR corroboration gate, a "reported
step band", and a cadence that "NARROWS" that band. Now describes the frozen
floor, the deleted gates, and why cadence is measured but deliberately not
consumed by the 1 Hz path.
388 tests pass.
Contributor
Author
|
Thanks — all three verified against the code and fixed in The vacuous test was the important one. Stale gate comment and the unreachable While in there I also corrected the file header, which you didn't flag but was describing the removed design — it still promised an HR corroboration gate, a "reported step band", and a cadence that "NARROWS" that band. Same class of known-false comment this PR exists to remove. 388 tests pass. |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/onehz/steps_test.dart (1)
420-430: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCreate an actual timestamp gap.
rows(d)creates aMotionMinutefor every minute. The sedentary rows split the elevated runs, so this test does not exercise missing-minute handling. Remove the rows between the two elevated pairs. Then the test will verify that a timestamp discontinuity cannot form one four-minute bout.Proposed fix
- // 4 elevated minutes total but never 3 adjacent, so none of it counts. + // Two elevated pairs are separated by eight missing timestamp rows. final d = List<double>.filled(60, sedDyn); d[10] = walkDyn; d[11] = walkDyn; d[20] = walkDyn; d[21] = walkDyn; - final m = dailyActiveMinutes(rows(d), personalDynFloorG: floorG); + final withCoverageGap = rows(d)..removeRange(12, 20); + final m = + dailyActiveMinutes(withCoverageGap, personalDynFloorG: floorG); expect(m.value!.activeMinutes, 0); + expect(m.value!.boutCount, 0);🤖 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/steps_test.dart` around lines 420 - 430, Update the test around rows(d) so it constructs MotionMinute data with the minutes between the two elevated pairs omitted, creating an actual timestamp discontinuity. Keep the elevated pairs at their existing timestamps and preserve the expectation that activeMinutes remains 0, ensuring the test verifies gaps do not stitch separate bouts together.
🤖 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.
Outside diff comments:
In `@test/onehz/steps_test.dart`:
- Around line 420-430: Update the test around rows(d) so it constructs
MotionMinute data with the minutes between the two elevated pairs omitted,
creating an actual timestamp discontinuity. Keep the elevated pairs at their
existing timestamps and preserve the expectation that activeMinutes remains 0,
ensuring the test verifies gaps do not stitch separate bouts together.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 58e25481-eac6-4c6f-9111-34613600bbfd
📒 Files selected for processing (2)
lib/src/onehz/motion/steps.darttest/onehz/steps_test.dart
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.
Why
A user reported 2,645 steps on a day they took under 400. Diagnosed on their real DB: that number was
23 active minutes × an assumed 115 spm. Both halves of the conversion are invalid at 1 Hz, and neither is fixable by retuning — so the conversion is removed, not recalibrated.Confirmed against that DB's own ground truth: in the one window where the 100 Hz pedometer and 1 Hz data overlap, HR ran 95→108 with dynAmp 0.31–0.40 g, and the real count was 11 steps in 3.1 min (3.5 spm) where the estimator would have assigned ~115 spm.
What changed
dailyStepEstimate→dailyActiveMinutes, returningDailyMovementEstimate: active minutes only. No steps, no cadence, no range. A contract test asserts the serialised surface can never regain a step/cadence key.Everything below was proven against 4 days of real 1 Hz substrate before being changed:
enrollmentDaysForFrozenFloor+shouldRefreezeFloor.The freeze proof — scaling one real day's
dynAmp:A recomputed floor reports the same number whether the user tripled their activity or did nothing.
Refuted — deliberately NOT built
Semantics corrected
The WHOOP 1 Hz accel field is a fused gravity vector, not acceleration. Across 269,486 real samples
‖a‖is p50 1.027 g with 0.030% above 1.3 g; during the single most vigorous minute of a day it measured 1.033 g ± 0.006, with 0 of 420 samples above 1.2 g.So
dynAmpmeasures how fast the wrist re-orients, and ENMO/MAD over this substrate reduce to~(1.03 − gRef)— a pure calibration artifact with zero signal. That is the true root cause of the 42,155-steps-at-gRef-0.97 / 0-at-1.02 collapse. Now documented on the type, replacing a comment that claimed gravity removal leaves "motion".Caveat
All measurements rest on 4 days of per-minute substrate — the export's retention limit. The direction of each result is unambiguous (zero-effect gates; a flat 37 across a 3× swing), but the CV figures are thin.
Test plan
dart analyzecleandart test→ 388 passing🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation