Repository navigation
fix(charge): score the breakdown against the baseline the ring rides - #2317
Merged
Merged
Conversation
The Charge page could show two different HRV baselines at once: one at the top from the engine, another on the "What shaped it" rows below it. Reported as 57 against 48 on one screen. Three folds of the same history disagreed because one argument was missing: TodayScoring.kt:41 foldHistory(..., hrvCfg, hrvBaselineEpoch) headline TodayScoring.kt:70 foldHistory(..., hrvCfg) driver rows ChargeBreakdownWiring.swift:38 the same, on Apple `noop.hrvBaselineEpoch` is written by Settings, Recalibrate HRV baseline. The engine discards every night dated before it; these folds kept the whole history. Until someone recalibrates the two agree, which is why this survived: the existing comment says as much, calling the whole-history fold "byte-identical to the engine's when no manual Recalibrate epoch is set, the common case". It is the uncommon case that shows two numbers for one metric. `chargeConfidenceTier` had the same gap and is fixed with it. That one reads ScoreConfidence off the baseline, so a whole-history fold could badge a scored day from a history the ring is not using. Both signatures take the epoch with a default of 0, and both foldHistory overloads delegate to the plain fold at <= 0, so a user who never recalibrated gets byte-identical behaviour. Each platform has a test asserting exactly that: labels, baselines and deltaPoints all unchanged when the epoch is absent. WHAT THIS DOES NOT SETTLE. Whether it resolves the report depends on whether that wearer has ever recalibrated, which is unanswered. If they have not, their epoch was already 0, these folds were already identical, and their 57 against 48 has another cause. Asked on the issue; the fix stands on its own either way, because the rows should score against the baseline the headline scores against. Verified: Android 6256 tests / 0 failures on a forced full run, doc lint clean. The Swift half is app-target and compiles in no local package, so it is parse-checked here and first type-checked by CI.
A fourth fold, found re-reading the change rather than by any tool.
`TodayView.watchScoreState` derives ScoreConfidence for the watch pill and its
comment says:
Same HRV-baseline gate the Charge engine uses
It was not. It folded the whole history while the engine folds from the
recalibration epoch, so after a Recalibrate the pill could read solid off nights
the ring has stopped using. The comment is now true.
Left alone deliberately: `WatchRecovery` on both platforms takes a value-only
history with no day keys, so an epoch cannot be applied there without changing
the shape of its input, and `AppModel`'s cycle-phase fold z-scores nights for a
different feature, where honouring a recalibration is a behaviour decision
rather than a correctness one.
Verified: doc lint clean. App-target, so parse-checked here and type-checked by
CI.
The Swift tests were written against the Kotlin type. They do not share a shape: Kotlin's ChargeDriver carries a `ChargeDriverLabel` enum and a numeric `baseline`, Swift's carries a `label` String and a pre-formatted `baselineText`, because that side formats for display inside the driver. So `$0.label == .heartRateVariability` and `.baseline` compiled nowhere, and `swiftc -parse` accepted all of it: both are type errors, not syntax. This is the app target, which builds in no local package here, so CI's macOS leg was the first thing that could say so, and it did. Rewritten to match: the label is matched as the string it is, and the assertion is that the two folds report DIFFERENT baselineText rather than an ordering between numbers that side does not expose. Different text is the observable proof that a different history was folded, which is the property under test.
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
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.
Refs #2315.
What
The Charge page could show two different HRV baselines at once: one at the top from the
engine, another on the "What shaped it" rows below it. Reported as 57 against 48 on the same
screen, with Coach offering a third number.
Three folds of the same history disagreed because one argument was missing:
noop.hrvBaselineEpochis written by Settings, Recalibrate HRV baseline. The engine dropsevery night dated before it; these folds kept the whole history. So the two agree until
somebody recalibrates, which is exactly why this survived review. The existing comment says
so out loud:
It is the uncommon case that puts two numbers for one metric on one page.
chargeConfidenceTierhad the same gap and is fixed alongside. That one readsScoreConfidenceoff the baseline, so a whole-history fold could badge a scored day from ahistory the ring is not using.
Nobody else can be affected
Both entry points take
hrvBaselineEpochwith a default of0, and bothfoldHistoryoverloads delegate to the plain fold at
<= 0. That is read from the source, not assumed:Each platform has a test asserting it: labels, baselines AND deltaPoints all identical when
the epoch is absent. A user who has never recalibrated sees byte-identical behaviour.
What this does not settle
Whether it resolves the report depends on whether that wearer has ever recalibrated, which is
still unanswered. If they have not, their epoch was already
0, these folds were alreadyidentical, and their 57 against 48 has a different cause. The question is on the issue.
The fix stands either way: the rows have to score against the baseline the headline scores
against, whatever caused this particular report.
Coach's number is a separate matter, not touched here. It computes its own plain 30-day mean
with no winsorization and none of the seed or trust gating (
CoachSuggestions.kt:94,CoachSuggestions.swift:97), so it will differ from both of the above for anyone with oneunusual night. Worth its own change, since routing it through
Baselinesis a behaviourdecision rather than a correctness one.
Tests
Three Kotlin, two Swift. The epoch fold discards pre-recalibration nights and lands on a
different baseline; an absent epoch changes nothing; and the confidence tier moves with the
same epoch, using a post-recalibration history that is still under the four-night seed gate
while the whole-history fold looks fully trusted.
Verification
python3 Tools/doc_comment_lint.py: cleanswiftc -parsechecked here and first type-checked by CI's
build (Strand, macOS), which also runs theChargeBreakdownWiringTestsadditions.