fix: HRV harmonic gate, 10s windows, rename to HR variability index#222
Merged
fix: HRV harmonic gate, 10s windows, rename to HR variability index#222
Conversation
…ity index The previous compute_hrv() produced inflated values (120-195ms) because: 1. Window-level IBI is NOT beat-to-beat IBI (not clinical RMSSD) 2. ~20% of windows locked to harmonics (474ms, 1458ms) undetected 3. Hampel filter couldn't catch clustered harmonic errors Fixes (informed by cardiologist, DSP engineer, and biostatistician review): - Rename: "HRV/RMSSD" → "HR variability index" (honest labeling) - Harmonic gate: trimmed-mean tracker rejects/corrects 2x/0.5x/3x IBIs before Hampel (18% tolerance per DSP expert) - 10s windows (was 30s): 3x more IBI samples (~59 vs ~19 per 5 min) - Gap-aware RMSSD: skip diffs across rejected-window gaps - Range gate: 5-100ms (was 5-200ms, per cardiologist recommendation) - HRValidator cap: 100ms (was 200ms) Pod validation: HRV now 20-70ms (was 120-195ms). Zero values >100ms. Closes #221 (now fixes) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR redefines the HRV field from an RMSSD sub-window metric to a non-clinical "HR variability index" computed from 10s windows (50% overlap) with a harmonic gate, Hampel filtering, gap-aware successive differences, and tightens the valid range to 5–100 ms. Docs and tests updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Input as Raw Samples
participant SHS as SHS Autocorrelation
participant Tracker as IBI Tracker
participant Gate as Harmonic Gate
participant Hampel as Hampel Filter
participant Diff as Gap-Aware Differences
participant Range as Range Gate (5–100 ms)
participant Output as HR Variability Index
Input->>SHS: 10s windows (50% overlap)
SHS->>Tracker: propose IBI estimates
Tracker->>Gate: provide tracker IBI
SHS->>Gate: raw IBI estimates
Gate->>Hampel: accepted IBIs (or corrected/discarded)
Hampel->>Diff: cleaned IBIs
Diff->>Range: compute successive-diff metric
Range->>Output: emit HR variability index or None
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Pull request overview
Updates the piezo-processor HRV calculation to a safer, more honest “HR variability index” by adding harmonic rejection/correction and increasing sub-window resolution to reduce inflated artifact-driven readings.
Changes:
- Reworked
compute_hrv()to use 10s overlapping windows, peak-only SHS scoring, harmonic gating, Hampel filtering, and gap-aware successive differences with a 5–100 ms validity gate. - Tightened HRV validation caps (200 ms → 100 ms) and updated unit tests to reflect the new upper bound.
- Updated docs to rename the metric and document the new processing pipeline and constraints.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/piezo-processor/main.py | Implements the new HR variability index pipeline (10s windows, harmonic gate, gap-aware diffs, tighter range gate). |
| modules/piezo-processor/test_main.py | Updates HRV-related assertions to the new 100 ms cap. |
| modules/common/calibration.py | Lowers HRValidator.HRV_HARD_MAX to 100 ms. |
| docs/piezo-processor.md | Renames HRV metric and documents the updated algorithm and parameters. |
| .claude/docs/trpc-api-architecture.md | Updates API field description to “HR variability index” (not clinical RMSSD) and range. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+571
to
+575
| gated.append((win_idx, accepted)) | ||
| tracker_history.append(accepted) | ||
|
|
||
| if len(gated) < _HRV_MIN_IBIS: | ||
| return None |
| raw_ibis: list = [] # (window_index, ibi_ms) | ||
| for idx, start in enumerate( | ||
| range(0, len(filtered) - sub_window + 1, step)): | ||
| chunk = filtered[start:start + sub_window] |
Comment on lines
+610
to
+613
| rmssd = float(np.sqrt(np.mean(sq_diffs))) | ||
| # Range gate: 5-100 ms. Window-level HRV above 100 ms is artifact | ||
| # even after harmonic correction (cardiologist recommendation). | ||
| return rmssd if 5 <= rmssd <= 100 else None |
Comment on lines
+746
to
+749
| # We expect an RMSSD; it may or may not match the exact input std | ||
| # but should be a plausible number | ||
| if hrv is not None: | ||
| assert 5 <= hrv <= 200 | ||
| assert 5 <= hrv <= 100 |
|
|
||
| def test_range_gate(self): | ||
| """RMSSD should be None or within 5-200 ms.""" | ||
| """RMSSD should be None or within 5-100 ms.""" |
Comment on lines
533
to
+537
| # Hard physiological caps (always enforced) | ||
| HR_HARD_MIN = 30.0 # bpm — athletes can drop this low (Circulation, AHA) | ||
| HR_HARD_MAX = 100.0 # bpm — above = tachycardia per AHA definition | ||
| HRV_HARD_MIN = 8.0 # ms — below = likely artifact | ||
| HRV_HARD_MAX = 200.0 # ms — BCG-derived >200 is artifact (Shaffer & Ginsberg 2017) | ||
| HRV_HARD_MAX = 100.0 # ms — window-level BCG HRV >100 is artifact (see #221) |
Comment on lines
+465
to
+469
| def _harmonic_gate(ibi_ms: float, tracker_ibi_ms: float) -> Optional[float]: | ||
| """Reject or correct IBIs that are harmonic multiples of the expected value. | ||
|
|
||
| Returns the accepted/corrected IBI in ms, or None to discard. | ||
| """ |
| sq_diffs: list = [] | ||
| for i in range(1, len(clean_ibis)): | ||
| gap = clean_indices[i] - clean_indices[i - 1] | ||
| if gap > _HRV_MAX_GAP_WINDOWS: |
- Rename local var rmssd → hrv_index for clarity - Fix gap threshold: >= not > (off-by-one) - Update HRValidator docstring to reflect 100ms cap - Rename all test comments from RMSSD to HRV index Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
🎉 This PR is included in version 1.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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.
Summary
The previous
compute_hrv()produced inflated, clinically meaningless values (120-195ms) labeled as "RMSSD." Three independent expert reviews (cardiologist, DSP engineer, biostatistician) identified the root causes and recommended fixes.Root causes
Fixes applied
Pod validation (2026-03-16)
Future work (#221)
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Tests