Repository navigation
hrv: say why the nightly summary reported nothing - #2129
Merged
Merged
Conversation
#2128 was filed against intended behaviour, on the strength of this line: hrv nightSummary reported=nil wholeNight=31.55ms deepOnly=28.56ms lastSWS=13.95ms nWin=87 nDeep=22 87 windows, 22 of them deep, three real means, and nothing reported. That reads as a value that went missing. It is the #1118 gate refusing an over-counted night on purpose, which is correct: RMSSD off beats that bank more beat-time than the wall clock is not a number worth keeping. The line now says so: hrv nightSummary reported=nil refused=crossSecondOverCount wholeNight=... The `hrv diag` row immediately above already carries `rrIntegrity`, which is what makes this worth fixing rather than shrugging at: that verdict is scored over the whole DAY while the gate runs per SESSION, so the two disagree exactly when it matters. One night in the field log reads `rrIntegrity=underCovered`, which is trustworthy, and still reported nothing, because a session inside it was not. Reading the adjacent line is precisely how the wrong conclusion got reached. The classification moves into `sessionRrVerdict` so the value gate and the explanation ask the same helper over the same beats. The gate's own comment insists a verdict must not describe a different set of beats than the number it gates, and two copies of it would eventually do that. Printed only when the night produced window means, so a night with no R-R at all explains itself and pays nothing. On a withheld night the cost is one O(n) filter and coverage pass per session. `sessionAvgHRV` warns against buying diagnostic detail here, but what it refuses is a SORT; `rrCoverage` is a single linear pass, so the warning is honoured rather than ignored. Kotlin 7 tests green, Swift 97. The new one on each side asserts an AGREEMENT rather than a fixed label: a withheld night must be exactly the night the traced verdict calls untrustworthy, so the explanation and the decision cannot drift apart. No scoring change. The gate keeps refusing exactly what it refused before; it just stops looking like a malfunction in the log.
Re-review. `deepHrvWindow` is a user setting, not a dev flag, and in that mode `avgHRVDaily` re-derives from `sessionHrvWindows` and never reads `s.avgHRV`. So the #1118 gate plays no part in that branch's nil, and the reason line would have named a gate that had nothing to do with it. The exact fault this line exists to correct: a diagnostic asserting a cause it did not verify. `nDeep` on the same line already covers that case, so it now stays quiet there. Worth noting separately, and not changed here because this PR alters no scoring: the deep branch has no over-count protection at all. A wearer who selects the deep-sleep HRV window gets a value built from beats the whole-night path would refuse.
Re-review. The shared helper added a declaration on both platforms, and both halves of this pair are INSIDE the parity ledger's scan, so it reported two new one-sided functions. Clearing that needs an authority migration the design deliberately makes hard, and the gate is path filtered to Tools/parity_*, so the drift would have sat here and failed some later unrelated PR instead. Fighting a governance migration to label a diagnostic is not a trade worth making, and it turns out not to be necessary. Inside sessionAvgHRV the only two paths to null are "no window yielded an RMSSD" and the gate. So windows-with-RMSSD beside a null value IS the gate, derived from two calls that already exist, with nothing restated that could later disagree with the thing it explains. That is a better answer than the helper was: no new symbol, no second copy of the classification, and no ledger debt. It now says `refused=overCount` rather than naming a verdict. sessionAvgHRV pins every over-count to CROSS_SECOND internally to avoid a sort, so the specific label was a distinction the code does not actually make, and printing it claimed more than was known. Ledger back to main's counts, 1870 Swift and 2440 Kotlin functions. Kotlin 7 tests green, Swift 7. Both now pin the same pair the trace reads rather than a label.
Restoring SleepStager on both platforms to exactly what main has. The extract-and-revert dropped the eight lines explaining why `collapsed` is passed the same figure as `coverage`, which is the reasoning this PR now leans on to justify saying `overCount` instead of naming a verdict. Neither stager is touched by this PR any more.
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.
What this is
#2128 was filed against intended behaviour. This does not change the behaviour; it makes the log stop looking like a malfunction.
The line that caused it:
87 windows, 22 of them deep, three real means, and nothing reported. That reads as a value that went missing. It is the #1118 gate refusing an over-counted night on purpose: RMSSD taken off beats that bank more beat-time than the wall clock they span is not a number worth keeping, and a blank is the right answer.
Now:
Why this is worth a change rather than a shrug
The
hrv diagrow immediately above already carriesrrIntegrity, so the natural response is that the information was there and the reader missed it. Mostly true, and it is how the wrong conclusion got reached.But that verdict is scored over the whole DAY, while the gate runs per SESSION, and they disagree exactly when it matters. One night in the field log reads
rrIntegrity=underCovered, which is trustworthy, and still reported nothing, because a session inside it was not. On that night the adjacent line does not explain the nil; it contradicts it.Shape
No new declarations. The reason is derived from two calls that already exist rather than by re-classifying the beats: inside
sessionAvgHRVthe only two paths to nil are "no window yielded an RMSSD" and the gate, so windows-with-RMSSD beside a nil value is the gate. Nothing is restated that could later disagree with the thing it explains.It says
refused=overCountrather than naming a verdict, becausesessionAvgHRVpins every over-count tocrossSecondOverCountinternally to avoid a sort. The specific label would be a distinction the code does not actually make, and printing it would claim more than is known.An earlier revision extracted a shared
sessionRrVerdicthelper. That was wrong for a reason worth recording: both halves of this pair are inside the parity ledger's scan, so it reported two new one-sided functions, and clearing that needs an authority migration the design deliberately makes hard. The governance gate is path-filtered toTools/parity_*, so the drift would have sat here and failed some later unrelated PR. Fighting a governance migration to label a diagnostic is not a trade worth making, and the derivation above is a better answer anyway. The ledger is back to main's counts: 1870 Swift and 2440 Kotlin functions.Printed only when the night produced window means, so a night with no R-R at all explains itself and costs nothing.
And not at all in deep-window mode, which re-review caught.
deepHrvWindowis a user setting rather than a dev flag, and that branch re-derives fromsessionHrvWindowswithout ever readings.avgHRV, so the #1118 gate plays no part in its nil. Naming it there would be the exact fault this PR exists to correct: a diagnostic asserting a cause it did not verify.nDeepon the same line already covers that case.Worth recording separately, and deliberately not changed here since this PR alters no scoring: the deep branch has no over-count protection at all. A wearer who selects the deep-sleep HRV window gets a value built from beats the whole-night path would refuse.
Type of change
How it was tested
Kotlin
HrvOverCountGateTest: 7 tests, 0 failures, forced with--rerun-tasks, XML timestamp checked against the clock. SwiftHrvOverCountGateTests+SleepStagerTests: 97 tests, 0 failures on Linux against a snapshot-enabled SQLite.The new test on each side pins the same pair the trace reads rather than a label: a withheld night must be exactly the night whose own beats were refused. Pinning the printed string would pass just as well if the explanation and the decision drifted apart.
Both platforms compile clean. Four files, and neither
SleepStageris touched.No scoring change. The gate refuses exactly what it refused before.
Related issues
Raised in #2128, which should be corrected rather than treated as a defect. The underlying refusal is #1118, still open.