fix(vital_signs): use circular variance for wrapped phases#595
Merged
ruvnet merged 1 commit intoMay 17, 2026
Merged
Conversation
process_frame computed arithmetic mean + variance on phase values from atan2(), which are wrapped to (-pi, pi]. Phases close across the +/-pi discontinuity produced ~pi^2 variance instead of ~1e-6, feeding wrap noise into the heart-rate FFT buffer. Replace inline math with a standard circular variance helper (1 - mean resultant length). Add 4 unit tests, one through the production path of process_frame. Closes ruvnet#593
ruvnet
approved these changes
May 17, 2026
ruvnet
left a comment
Owner
There was a problem hiding this comment.
Reviewed end-to-end:
- Math is correct:
1 - RwhereR = sqrt(sin_sum^2 + cos_sum^2) / nis the standard Mardia circular variance. - Bounded to
[0, 1]instead of[0, ~pi^2], which is fine because the heartbeat FFT cares about relative spectral peaks (confirmed in the PR description). - The regression test
test_phase_variance_handles_wraparoundis particularly nice — it asserts both the new bound AND that the old buggy formula on the same input produces ~pi^2, which makes a regression to the linear computation explicitly fail. test_process_frame_handles_wrapped_phasesis the end-to-end check that anyone can run against main to observe the bug and against this branch to observe the fix. Excellent reproducer.- Identical-phase and opposite-phase edge cases covered.
- Scoped to a single file. Well-commented. Closes #593.
Approving. Merging with squash.
This was referenced May 17, 2026
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.
The heartbeat feature in process_frame was computed as arithmetic mean + arithmetic variance on phases that come from atan2() and are wrapped to (-pi, pi]. Two phases that straddle +/-pi are physically aligned but the linear formula treats them as ~2*pi apart, so you get ~pi^2 (~9.87) instead of ~1e-6. That noise rides into the heart-rate FFT buffer every time a subcarrier's phase crosses the wrap.
Swapped the inline math for a standard circular variance (1 - R, where R is the mean resultant length). The value is bounded [0, 1] instead of [0, ~pi^2] now, but the heartbeat FFT cares about relative spectral peaks, not absolute scale.
Tests:
4 new tests: one drives
process_frameend-to-end and inspects the heartbeat_buffer; the other three hit the helper with wraparound, opposite, and identical inputs.Related to the jumpy vitals in #519 and the +/-15 bpm jitter mentioned in #485. #485 wraps VitalSignDetector with an external pipeline but keeps it as the low-confidence fallback - this fix is complementary.
Closes #593