Repository navigation
feat: add sleep-aware steps and physiological day cycles - #1572
Conversation
5fcd73e to
4a171e6
Compare
…analyzeRecentOnCpu (#1696) analyzeRecentOnCpu had SEVENTEEN bytes of headroom: 61,518 against the 61,535-byte JaCoCo budget its own guard pins. Not a margin, a wall - and one that announces itself at test time rather than at review time, so the next person to touch that method for any reason hit it with no hint it had been full since before they arrived. analyzeRecentOnCpu instrumented Code length 61,518 -> 54,087 margin to the JVM's 65,535 method limit 4,017 -> 11,448 7,431 bytes freed by lifting the per-day skin-temp, SpO2 and off-wrist reads into a private suspend helper. IT IS A MOVE, AND THE DIFF PROVES IT. All 44 removed lines reappear verbatim in the additions; the extra lines are only the signature, the holder, the doc and the call site re-binding the same six names it used inline. Side-effect order is unchanged - the four per-owner memo maps are passed by reference and mutated in the same sequence, so WHEN the WHOOP 4.0 ADC anchor is learned and WHAT FROM are identical. The call sits inside the same prep timing bracket, so the analyzeRecent cost split still attributes these reads exactly as before. WHY EXTRACT RATHER THAN RAISE THE BUDGET. The guard exists because JaCoCo instrumentation already blew the JVM method limit once (bhelm#102), and this file already answered the question: persistFitnessVitalityAndSteps was extracted for exactly this reason, with a second test pinning that it stays extracted and at its required call site. Raising a budget to fit a change is how the method got to 17 bytes. WHY THIS BLOCK. Best ratio of bytes freed to boundary crossed - six values out against eleven in, all already local - and self-contained. The full 115-line read region was the blunt alternative: sixteen values crossing instead of six, for a refactor whose only job is to make room. THE BUDGET IS NOW A RATCHET, 61,535 -> 56,000. Leaving it would have left 7,448 bytes to be spent the same silent way, which is the mechanism this removes rather than merely resets. The ~1.9 K margin is deliberate: enough for an ordinary change (the pass-1 sliding read windows need about 825), small enough that a large regression fails at the test rather than at the 64 KB wall. 56,000 rather than the 55,000 first floated, because 55,000 would have left 88 bytes after the wiring and rebuilt the wall inside one PR. The comment states the discipline: lower it whenever an extraction frees space, never raise it to fit a change. persistFitnessVitalityAndSteps is left at 12,000 against 6,415 - slack, but not creeping, and tightening a method nobody is pressing against would be tightening for its own sake. MARKED DELIBERATELY ONE-SIDED. The Swift engine keeps this block inline, because the constraint is a JVM one and Swift has neither the limit nor the guard. The doc says do not mirror it, so a parity audit that finds a Kotlin helper with no Swift counterpart has found the intended state rather than a gap. REGRESSION SURFACE, CHECKED RATHER THAN ASSERTED. The three open PRs touching this file (#1605, #1587, #1572) conflict identically against main and against this branch - 44, 4 and 8 files, with IntelligenceEngine.kt already among #1605's against main. This adds no conflict. Nor can the ratchet block them: all three pass CI today, which is itself proof they add nothing to analyzeRecentOnCpu, since any addition would already fail at 17 bytes. They go from 17 bytes of room to 1,913. Android 4604 tests 0 failures, the same count as main. doc_comment_lint OK, i18n green. No Swift touched, so no app-build exposure.
cf2f7b5 to
3559bb7
Compare
|
CI status after the rebase: every check except Android The remaining failure is unrelated to this PR and reproduces unchanged on current The time-dependent test was introduced by #1631. Its fixture is fixed at |
1a7fc17 to
6d63125
Compare
|
@ryanbr I have rebased this PR onto the latest When you have a chance, could you share any concerns you have about the approach or scope? I would be happy to make the review easier—for example by splitting the change into smaller PRs, adding a focused architecture/data-flow summary, highlighting the highest-risk parts, or providing additional tests and field evidence. Let me know what would be most useful to you. |
ryanbr
left a comment
ryanbr
left a comment
There was a problem hiding this comment.
Really solid work — the field evidence, the clicker ground truth and the deliberate separation of DayCycleResolver from step classification all stand out. Core analytics check out as genuine twins: gait constants identical (3/4/5/6), Count the same nine fields, classifyForCycle / cycleWindows / ownerSegmentsFromCoverage matched, default SLEEP_ONSET and the Settings escape hatch on both platforms.
One thing I'd want fixed before merge, plus one cleanup.
Apple applying(...) drops two columns
Strand/Data/DayCycleIntelligenceIntegration.swift:273 respells DailyMetric field by field and ends at avgSdnn. The branch already carries skinTempC (MetricsCache line 107) and sleepHrOnly (line 109) — neither appears anywhere in that file. Both default to nil, so it compiles and silently nils them.
It is live: IntelligenceEngine.swift:1844 calls it for every scored day, so the nightly absolute skin temperature and the HR-only staging flag are wiped on any day the step cycle touches.
Kotlin is fine — its twin is daily.copy(steps = ...), which carries untouched fields for free. Swift structs have no copy(), so every rebuild loses columns by omission; IntelligenceEngine.swift warns about exactly this above its own with(...) helpers. Two extra arguments and it's done.
Tested but never called
advanceBoundary (plus its private isPlausibleMainSleep), newCycleCount, accumulateCyclePage, finishCycleCount, and stepsInCycle have no production callers — counting runs through accumulator.acceptPage. The tests read as coverage for logic that never executes, and it is the surface most likely to drift from the path that does. Worth deleting or wiring.
Not a defect, just naming the decision
Default SLEEP_ONSET re-buckets everyone's day on upgrade, and deliberately only for steps and in-progress Effort — calories, persisted Effort, HR charts and workouts stay on calendar days. So Today will show two day definitions side by side. That is documented in the description and argued well; flagging it as the thing to decide on rather than a problem with the code.
Nothing else blocking from me. CI is green across Android and every Swift package job.
|
Implemented the review findings and the follow-up product decision:
The typical midnight sleeper should see little difference; the intended behavior change is for activity after midnight before a later bedtime. |
a53d2ed to
21f9b8b
Compare
|
Follow-up fixes are now pushed in The physiological cycle now consistently covers steps, completed/live Effort, calories, HR timelines, and workout assignment/counts. The update also fixes the iOS published-strain mismatch, reloads both iOS Today variants when the cycle mode changes, keeps adjacent HR windows end-exclusive, and reads HR/workouts across all registered and archived WHOOP sources plus canonical history. Persisted all-source workouts and fresh detections are deduplicated before the cycle count is materialized; Coupled now consumes that value. Verification:
I also updated the PR description so it reflects the expanded day-cycle consumers. |
…low-up) Re-review of the previous commit, which claimed to add "the one resolver case Apple was missing". That claim was wrong and the test was vacuous: it asserted a 23:00 onset resolves to 2 * 86_400, and the existing `testSleepOnsetCycleStaysOpenAcrossMidnight` already asserted exactly that value through exactly that branch from a 20:00 onset. I had matched case COUNTS across the platforms and called it parity, which is not the same thing as matching what they reach. The branch neither platform reached is the other one. `fallbackMidnightAfter` picks `floorDiv(minimum, day) * day`, which is at or BELOW `minimum` always, so `atMidnight >= minimum` is true only on exact equality — when onset sits precisely 18 h before a midnight. Every case on both platforms used an onset that rolls past it. So a `>=` quietly weakened to `>` would have moved that boundary a full day with nothing failing. Verified rather than argued: mutating `>=` to `>` fails the new case and ONLY the new case; the five existing ones, including the 23:00 roll, stay green. Both platforms now pin both branches, with the rolling case kept at an onset distinct from the direct one so the pair are not the same test twice.
…follow-up) Re-review pass three. Every day-cycle case on both platforms passed a zero timezone offset, so the offset arithmetic — the part that decides which LOCAL day a boundary lands on — was pinned by nothing at all. That is the one input this repo has already been bitten by, when recent nights re-bucketed on travel. The two implementations do agree today: Swift's `Int(floor(Double(local) / Double(day)))` and Kotlin's `Math.floorDiv(local, day)` return identical answers across every offset from UTC-12 to UTC+13 and every onset hour across two days. Nothing recorded that agreement, though, and the resolver now carries a case that does. 06:00 local at UTC-5 is 11:00 UTC; plus 18 h is 05:00 UTC the next day, which IS local midnight there, so the direct branch takes it and the answer is 104_400 — 29 h UTC, 00:00 local. A resolver that floored to UTC midnight would answer 86_400 and be a full day out for a third of the planet. Verified rather than argued, as with the previous commit: dropping the offset term so the resolver floors to UTC midnight fails the new case and ONLY the new case. All six existing cases stay green, which is the evidence that they were blind to the offset entirely.
…hed (#1572 follow-up) Cleanup on top of #1572, found while reviewing it. No window boundary changes. `DayCycleResolver.activeWindow` took a `reliableAwakeCoverage` flag that neither platform's body read and every call site passed a constant — residue of a gate that was dropped while the parameter was not. Two Kotlin cases are why it survived: their names asserted a coverage distinction the code never made, and both passed either way. Renamed to what they actually pin. Then two branches that existed but were pinned by nothing: - `fallbackMidnightAfter` picks `floorDiv(minimum, day) * day`, always at or below `minimum`, so the direct branch is reachable only on exact equality — onset precisely 18 h before a midnight. Every case on both platforms used an onset that rolls past it, so `>=` weakened to `>` would have moved that boundary a day with nothing failing. - Every day-cycle case passed a ZERO timezone offset, so the arithmetic deciding which LOCAL day a boundary lands on was unpinned — the input this repo has already been bitten by, when nights re-bucketed on travel. 06:00 local at UTC-5 resolves to 104_400: 29 h UTC, 00:00 local. Both were verified by mutation rather than argument: each new case fails alone under the corresponding break, while every pre-existing case stays green.
Two more the compile gate surfaced once the first layer cleared. `DailyMetric.fillingNilFields` and `takingSleepFields` sit in a `private extension`, which even `@testable import` cannot see, so the cases exercising them could never have compiled. Widened to internal, still module-confined. They stayed private because nothing demanded otherwise: the tests calling them have never run. `DayCycleRecoveryTests` referenced `UserProfile` without importing StrandAnalytics, which arrived with the day-cycle work in #1572 and has been red since. Both are the same story as the rest of this branch. StrandTests runs only in the on-demand app-build, so a test file can be merged in a state where it does not compile and stay that way indefinitely.
…lost its context menu Two losses from the v17 uplift, both found by looking for them rather than by a failing build. 1. `lightPassMerged` did not respell `sleepHrOnly`, a field upstream added in ryanbr#1572. A Swift struct has no `copy()`, so an omitted field is rebuilt as nil — and the dailyMetric upsert takes `excluded.<field>` unconditionally, so every light pass wrote nil over the stored night. Upstream hit this exact trap in their own rebuild and lost `skinTempC`/`sleepHrOnly`; the fork then hit it again because the existing test asserts field-by-field and cannot fail for a field nobody remembered to add. The new test compares against a stored row with every optional populated and asserts NOTHING scored came back nil, naming any dropped field in the failure message. It fails on the next such field instead of the next incident. 2. `CoachView`'s assistant bubble lost upstream's K8 context menu (Copy / Share / Save to Journal) when the fork's attribution VStack was composed around upstream's Markdown. The giveaway was `saveAdvice` sitting defined-but-uncalled: dead code compiles clean, so the build stayed green and the feature disappeared silently. Re-attached to the VStack so the menu covers the attribution line too. Both app targets build; 2,135/2,137 app-target tests pass (the 2 failures are upstream's own date-sensitive tests, which fail identically on a clean v11.6.0 checkout).
…lost its context menu Two losses from the v17 uplift, both found by looking for them rather than by a failing build. 1. `lightPassMerged` did not respell `sleepHrOnly`, a field upstream added in ryanbr#1572. A Swift struct has no `copy()`, so an omitted field is rebuilt as nil — and the dailyMetric upsert takes `excluded.<field>` unconditionally, so every light pass wrote nil over the stored night. Upstream hit this exact trap in their own rebuild and lost `skinTempC`/`sleepHrOnly`; the fork then hit it again because the existing test asserts field-by-field and cannot fail for a field nobody remembered to add. The new test compares against a stored row with every optional populated and asserts NOTHING scored came back nil, naming any dropped field in the failure message. It fails on the next such field instead of the next incident. 2. `CoachView`'s assistant bubble lost upstream's K8 context menu (Copy / Share / Save to Journal) when the fork's attribution VStack was composed around upstream's Markdown. The giveaway was `saveAdvice` sitting defined-but-uncalled: dead code compiles clean, so the build stayed green and the feature disappeared silently. Re-attached to the VStack so the menu covers the attribution line too. Both app targets build; 2,135/2,137 app-target tests pass (the 2 failures are upstream's own date-sensitive tests, which fail identically on a clean v11.6.0 checkout).
2 participants
What this PR does
Adds sleep-aware step counting and a reusable physiological day-cycle abstraction on Android and Apple platforms.
This fixes a substantial accuracy bug in the previous NOOP counter. It could overcount motion very aggressively: we observed hundreds of steps accumulating while the wearer was asleep, including a morning where NOOP showed roughly 400 steps despite no walking during the sleep window and WHOOP still showed 0. Short early-morning movement could also be inflated dramatically. These were not harmless display differences; isolated counter ticks, sleep motion, implausible jumps, and calendar-boundary handling were being interpreted as real walking.
The default daily cycle now begins at the onset of the detected main sleep rather than blindly resetting at 00:00. Naps do not create a new cycle. If sleep is missing or unreliable, the cycle falls back to local midnight, and an active cycle is capped so a missed night cannot produce a 48-hour "day". Users can opt back into a conventional 00:00 calendar day in Settings.
The practical motivation is my late sleep schedule: I often go to bed around 02:00. A hard reset at midnight then splits one continuous waking period in the middle of my evening, resets my visible step total two hours before bed, and assigns my after-midnight household activity to a different "day" even though I have not slept yet. Main-sleep onset is a much more meaningful boundary for that pattern, while the explicit 00:00 option preserves conventional calendar-day behavior for users who prefer it.
Steps are counted from persisted strap history, not only while a screen is open. The counter:
The day-cycle code is intentionally separate from step classification.
DayCycleResolverand its persisted mode can be reused by other additive/live daily features. This PR already uses it for steps and in-progress Effort; future metrics can share the same boundary semantics instead of inventing their own reset logic.Current day-cycle consumers
Field evidence
This was developed against timestamped step-debug exports plus manual ground truth collected with a mechanical clicker. The dataset includes continuous walking, mixed walking/jogging, household work, stairs, quiet sitting, sleep, and explicitly marked zero-step car journeys.
Two representative controlled checks:
+1.0%); the concurrently worn Amazfit advanced by 681 (-16.7%).+3.3%); Amazfit advanced by 698 (-3.3%); WHOOP eventually advanced by about 650 (-10.0%).WHOOP also revised settled-looking totals after the walk without further movement (observed
3109 → 3260and3771 → 3910), so comparisons used its later value where available. Across the controlled sessions, the new NOOP candidate was generally closer to the clicker than WHOOP, while the final full-day totals were NOOP 6143 versus WHOOP 5500. That supports the hypothesis that this implementation can outperform WHOOP for the tested wearer and activities; it is deliberately not presented as a population-wide benchmark yet.The sleep-aware cycle and tick filtering substantially reduced the observed wake-up false total: an earlier build accumulated roughly 400 steps without overnight walking, while the latest overnight test displayed 20 at wake-up. The detailed trace for that latest night later resolved 52 accepted ticks before 09:30 against user-confirmed ground truth of zero steps. The physiological day boundary itself behaved correctly: it reset at the detected 01:29 main-sleep onset, did not split the waking period at midnight, remained stable overnight, and assigned post-wake movement to the new cycle. The remaining false positives belong to step-phase classification around fragmented sleep/wake context, not cycle resolution; they are preserved in the local research corpus for follow-up algorithm work without changing the reusable cycle API.
Type of change
How it was tested
:app:testFullDebugUnitTestpassed with at most two Gradle workers.StrandAnalyticsandWhoopStore: https://github.com/bhelm/noop/actions/runs/32666854693OK no NEW parity ledger findingserrors=0Design notes
IntelligenceEngine, avoiding process-global cross-store interference.preserve/replaceresult so an unread namespace cannot be mistaken for an authoritative empty one.Checklist
Related issues
This PR is based on the local step-counting investigation and field dataset; it does not close an existing issue.