fix irregular-rhythm window diffs + kill dup clamp helper - #58
Closed
abdulsaheel wants to merge 2 commits into
Closed
fix irregular-rhythm window diffs + kill dup clamp helper#58abdulsaheel wants to merge 2 commits into
abdulsaheel wants to merge 2 commits into
Conversation
…amp helper per-window sustained-irregularity check was diffing the compacted nn array positionally, so a dropped artifact beat inside a window manufactured a spurious jump just like the aggregate path used to before 6e11781 -- carry adjacency through into the window pass now. also killed the free clamp() helper in util.dart since it's just math.max(lo, math.min(hi,x)) and num.clamp already does that -- swapped all call sites to the native method. replaced a hand-rolled stddev in cardio_stager's _windowSdnn with the shared stddev() helper. fixed a stale doc comment in onehz.dart pointing at a deleted file, and added a quick-start snippet to the readme.
Reviewer's GuideThe PR fixes windowed irregular-rhythm detection by carrying original-series adjacency through beat compaction, removes and replaces the duplicated clamp helper throughout the package, reuses shared standard-deviation logic in cardio staging, and refreshes documentation with corrected package guidance and a runnable quick start. Sequence diagram for artifact-aware irregular-rhythm window detectionsequenceDiagram
participant Caller
participant Screen as irregularBeatScreen
participant Windows as _sustainedAcrossWindows
participant Stddev as stddev
Caller->>Screen: irregularBeatScreen(rrMs, rrTimesMs, ...)
Screen->>Screen: Build keep, nn, and nnAdjacent
Screen->>Windows: _sustainedAcrossWindows(nn, nnTimes, nnAdjacent, ...)
Windows->>Windows: Group beats into time windows
Windows->>Windows: Keep diffs only when bucketAdjacent[i]
Windows->>Stddev: stddev(diffs)
Windows->>Stddev: stddev(bucket)
Stddev-->>Windows: Window variability
Windows-->>Screen: Sustained irregularity result
Screen-->>Caller: Metric<IrregularRhythm>
Flow diagram for preserving beat adjacency across artifact removalflowchart LR
A[Original rrMs] --> B{Beat passes keep filter?}
B -->|yes| C[Compacted nn beat]
B -->|no| D[Dropped artifact beat]
C --> E[Record nnAdjacent]
D --> E
E --> F[Build time window bucket]
F --> G{Adjacent to prior original beat?}
G -->|yes| H[Include successive difference]
G -->|no| I[Skip difference across artifact]
H --> J[Window stddev and irregularity flag]
I --> J
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="lib/src/onehz/clinical/cosinor.dart" line_range="127-134" />
<code_context>
ssRes += (y[i] - fit) * (y[i] - fit);
}
- final r2 = ssTot == 0 ? 0.0 : clamp(1 - ssRes / ssTot, 0, 1);
+ final double r2 = ssTot == 0 ? 0.0 : (1 - ssRes / ssTot).clamp(0, 1);
// Adjusted for the 3 fitted parameters (M, β, γ). Confidence MUST come from
// the adjusted value: the raw R² of a 3-parameter fit is upward-biased
</code_context>
<issue_to_address>
**issue (bug_risk):** The native `num.clamp` API returns `num`, so these replacements produce static type errors where the result is assigned to a `double` or returned from a function declared to return `double`; the package no longer analyzes or compiles.
**Suggested fix:** Call `.toDouble()` after `clamp`, or add a typed helper that returns `double`.
```suggestion
final double r2 = ssTot == 0 ? 0.0 : (1 - ssRes / ssTot).clamp(0, 1).toDouble();
// Adjusted for the 3 fitted parameters (M, β, γ). Confidence MUST come from
// the adjusted value: the raw R² of a 3-parameter fit is upward-biased
// (E[R²] = 2/(n−1) under the null), so a handful of noise points used to
// score confidence 0.95 at tier HIGH.
final double r2Adj = (1 - (1 - r2) * (n - 1) / (n - 3)).clamp(0, 1).toDouble();
final conf = r2Adj.clamp(0.1, 0.95).toDouble();
```
</issue_to_address>
### Comment 2
<location path="README.md" line_range="73-74" />
<code_context>
+## Quick start
+
+```dart
+import 'package:openstrap_analytics/onehz.dart';
+
+// nnMs: cleaned beat-to-beat RR intervals in ms (see foundations/rr_correction.dart
+// for turning raw RR into this). nnTimesMs: elapsed ms per beat, same length.
+final Metric<HrvTime> hrv = hrvTime(nnMs, nnTimesMs: nnTimesMs, artifactFraction: 0.04);
+if (hrv.value != null) {
+ print('RMSSD ${hrv.value!.rmssd} ms (confidence ${hrv.confidence}, tier ${hrv.tier})');
</code_context>
<issue_to_address>
**issue:** The advertised runnable quick-start snippet references `nnMs` and `nnTimesMs` without declaring or initializing either variable, so copying the example fails to compile immediately.
**Triggers:** When a user copies the quick-start example as provided.
**Suggested fix:** Declare concrete sample lists in the snippet, or show how to obtain them from `RrCorrectionResult`.
```suggestion
final nnMs = <double>[800, 810, 795, 805];
final nnTimesMs = <double>[0, 800, 1610, 2405];
```
</issue_to_address>
### Comment 3
<location path="README.md" line_range="73-75" />
<code_context>
+```dart
+import 'package:openstrap_analytics/onehz.dart';
+
+// nnMs: cleaned beat-to-beat RR intervals in ms (see foundations/rr_correction.dart
+// for turning raw RR into this). nnTimesMs: elapsed ms per beat, same length.
+final Metric<HrvTime> hrv = hrvTime(nnMs, nnTimesMs: nnTimesMs, artifactFraction: 0.04);
+if (hrv.value != null) {
+ print('RMSSD ${hrv.value!.rmssd} ms (confidence ${hrv.confidence}, tier ${hrv.tier})');
</code_context>
<issue_to_address>
**issue (bug_risk):** The quick-start comment says `nnTimesMs` contains elapsed milliseconds per beat, but `hrvTime` expects cumulative beat timestamps; passing RR durations makes its gap check compare differences between durations rather than elapsed beat times and incorrectly retains or drops successive-difference pairs.
**Triggers:** When a caller follows the documented meaning and supplies one elapsed-duration value per beat.
**Suggested fix:** Describe `nnTimesMs` as cumulative beat times in milliseconds and provide a timestamp construction example.
</issue_to_address>Sourcery assessment
Approval pending. 3 findings to address first.
Blocking findings: lib/src/onehz/clinical/cosinor.dart:134, README.md:74, README.md:75
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+127
to
+134
| final double r2 = ssTot == 0 ? 0.0 : (1 - ssRes / ssTot).clamp(0, 1); | ||
| // Adjusted for the 3 fitted parameters (M, β, γ). Confidence MUST come from | ||
| // the adjusted value: the raw R² of a 3-parameter fit is upward-biased | ||
| // (E[R²] = 2/(n−1) under the null), so a handful of noise points used to | ||
| // score confidence 0.95 at tier HIGH. | ||
| final r2Adj = clamp(1 - (1 - r2) * (n - 1) / (n - 3), 0, 1); | ||
| final double r2Adj = (1 - (1 - r2) * (n - 1) / (n - 3)).clamp(0, 1); | ||
|
|
||
| final conf = clamp(r2Adj, 0.1, 0.95); | ||
| final conf = r2Adj.clamp(0.1, 0.95); |
There was a problem hiding this comment.
issue (bug_risk): The native num.clamp API returns num, so these replacements produce static type errors where the result is assigned to a double or returned from a function declared to return double; the package no longer analyzes or compiles.
Suggested fix: Call .toDouble() after clamp, or add a typed helper that returns double.
Suggested change
| final double r2 = ssTot == 0 ? 0.0 : (1 - ssRes / ssTot).clamp(0, 1); | |
| // Adjusted for the 3 fitted parameters (M, β, γ). Confidence MUST come from | |
| // the adjusted value: the raw R² of a 3-parameter fit is upward-biased | |
| // (E[R²] = 2/(n−1) under the null), so a handful of noise points used to | |
| // score confidence 0.95 at tier HIGH. | |
| final r2Adj = clamp(1 - (1 - r2) * (n - 1) / (n - 3), 0, 1); | |
| final double r2Adj = (1 - (1 - r2) * (n - 1) / (n - 3)).clamp(0, 1); | |
| final conf = clamp(r2Adj, 0.1, 0.95); | |
| final conf = r2Adj.clamp(0.1, 0.95); | |
| final double r2 = ssTot == 0 ? 0.0 : (1 - ssRes / ssTot).clamp(0, 1).toDouble(); | |
| // Adjusted for the 3 fitted parameters (M, β, γ). Confidence MUST come from | |
| // the adjusted value: the raw R² of a 3-parameter fit is upward-biased | |
| // (E[R²] = 2/(n−1) under the null), so a handful of noise points used to | |
| // score confidence 0.95 at tier HIGH. | |
| final double r2Adj = (1 - (1 - r2) * (n - 1) / (n - 3)).clamp(0, 1).toDouble(); | |
| final conf = r2Adj.clamp(0.1, 0.95).toDouble(); |
…cumulative not elapsed
Contributor
Author
|
squashed into #59 for one clean review — closing this round. |
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
1 participant
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.
found the per-window irregular-rhythm check was diffing the compacted beat array positionally instead of skipping across a dropped artifact beat -- same bug the aggregate path already got fixed for in 6e11781, just not carried into the window pass. carries adjacency info through now.
also:
clamp()in util.dart, it's a dupe ofnum.clamp-- swapped ~70 call sites over_windowSdnnin cardio_stager now calls the sharedstddev()instead of hand-rolling itran
dart analyze --fatal-infosand the full test suite, both clean.Summary by Sourcery
Preserve beat adjacency in irregular-rhythm window analysis and consolidate shared numeric utilities.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: