fix(consensus)!: clamp codec/duplex scalar depth+error tags to fgbio's Short ceiling - #552
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
WalkthroughPer-base depths and errors now cap at fgbio’s ChangesDepth saturation
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #552 +/- ##
==========================================
+ Coverage 92.84% 92.89% +0.05%
==========================================
Files 166 167 +1
Lines 102064 102777 +713
==========================================
+ Hits 94765 95479 +714
+ Misses 7299 7298 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
nh13
force-pushed
the
nh/fix-consensus-depth-clamp-parity
branch
from
July 13, 2026 19:34
80292cb to
14a1cd8
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
nh13
force-pushed
the
nh/fix-consensus-depth-clamp-parity
branch
from
July 16, 2026 15:31
14a1cd8 to
c3131c5
Compare
nh13
changed the title
nh13
force-pushed
the
nh/fix-consensus-depth-clamp-parity
branch
from
July 16, 2026 19:46
c3131c5 to
11dff2d
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fgumi-consensus/src/codec_caller.rs`:
- Around line 3224-3330: Add generated fgbio-oracle coverage for the saturation
contract in crates/fgumi-consensus/src/codec_caller.rs lines 3224-3330: replace
or supplement the hand-authored formula tests with a programmatically generated
deep CODEC fixture and assert emitted scalar and per-base tags match fgbio. Add
the equivalent generated fixture in crates/fgumi-consensus/src/duplex_caller.rs
lines 5028-5176, covering mixed strand saturation and capped error numerators;
both sites must assert identity with the fgbio baseline or document any
intentional divergence.
- Around line 1350-1364: Change the combined-depth accumulation in the consensus
error-rate calculation around total_depths and total_bases to use i64, matching
the duplex implementation. Ensure the per-base depth values are converted before
summing and adjust the division types as needed so deeply covered consensuses
cannot overflow while preserving the existing zero-depth behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b9a82a46-0dec-4261-80c6-d6f2a11ac53b
📒 Files selected for processing (3)
crates/fgumi-consensus/src/caller.rscrates/fgumi-consensus/src/codec_caller.rscrates/fgumi-consensus/src/duplex_caller.rs
…s Short ceiling fgbio stores per-base consensus depth AND error counts as `Array[Short]` capped at `Short.MaxValue` (32767) and derives every scalar depth/error-rate tag from those capped arrays. fgumi keeps per-base depth and errors in `u16` (up to 65535). The simplex (vanilla) caller already matches fgbio by clamping both depths and errors at push, but the codec and duplex callers emitted the scalar `aD`/`bD`/`cD` (plus the `aM`/`bM`/`cM` minima and the `aE`/`bE`/`cE` error rates) uncapped, so they diverged from fgbio on families deeper than 32767 reads. The three callers were therefore inconsistent with each other and with fgbio. Clamp each per-base depth to the Short ceiling before `max`/`min`/sum, via a shared `clamp_per_base_to_fgbio_short` helper. For the combined duplex/codec `cD`/`cM`, cap each strand's per-base value *before* summing, exactly as fgbio's `totalDepths(i) = min(abD_i, 32767) + min(baD_i, 32767)`, so a mixed pair where only one strand saturates yields the correct intermediate value (e.g. 62767), not an uncapped sum. Cap the error-rate numerators the same way, so `aE`/`bE`/`cE` are summed from per-base errors capped at the ceiling, matching fgbio's capped `errors` `Array[Short]` -- both the numerator and denominator of the rate now derive from Short-capped per-base values. Only the emitted tags are capped; the unclamped depth is still used for the min-reads threshold, so no behavior is lost. Adds a helper case table, duplex/codec depth-saturation tests covering the intermediate `cD` case, and duplex/codec error-rate numerator-cap tests, alongside the existing vanilla saturation test.
nh13
force-pushed
the
nh/fix-consensus-depth-clamp-parity
branch
from
July 17, 2026 14:16
11dff2d to
e268c6c
Compare
Merged
nh13
added a commit
that referenced
this pull request
Jul 17, 2026
…tore The CODEC caller's per-base combined duplex error was `ea + eb` (and the disagreement-branch sums) on `u16`. For very deep families where both strands carry a high per-base error count, `40000 + 40000 = 80000` exceeds `u16::MAX`, which panics in debug and wraps in release BEFORE `cE` clamps it. This is the error-count analogue of the depth cap-before-sum fix in #552 (which only capped the per-base depth); the error sum was left uncapped. Sum each per-base error in `u32` and saturate at fgbio's `Short` ceiling via a shared `clamp_duplex_error_to_fgbio_short` helper, matching fgbio's per-base `errors` `Array[Short]` (capped at 32767 at storage). The downstream `cE` numerator re-clamps to the same ceiling, so this changes only the stored intermediate, never the emitted tag. The duplex caller's analogous `error_at_i` already sums in `i32` and clamps to `i16::MAX`, so it was unaffected. Adds `test_duplex_per_base_error_caps_before_summing`, the error-array companion of the existing per-base depth cap-before-sum test, driving `build_duplex_consensus_from_padded` with both strands agreeing and each carrying a per-base error above the ceiling (which panicked in debug before the fix).
This was referenced Jul 21, 2026
This branch was previously deployed
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.
What
Clamp the scalar consensus depth tags (
aD/bD/cDand theaM/bM/cMminima) and the error-rate tags (aE/bE/cE— both the depth denominators and the error numerators) to fgbio'sShortceiling (i16::MAX= 32767) in the codec and duplex callers, matching what the vanilla (simplex) caller already does and what fgbio emits. This makes all three consensus callers consistent with each other and bit-identical to fgbio.Why
fgbio stores per-base consensus depth and per-base error counts as
Array[Short]capped atShort.MaxValueand derives every scalar depth/error-rate tag from those capped arrays. fgumi keeps per-base depth and errors inu16. The vanilla caller already matches fgbio by clamping both depths and errors at push (with a test), andbase_builder.rsdocuments the intent to clamp "at tag emission" — but the codec and duplex callers emitted the scalar depth and error-rate tags uncapped, so they diverged from fgbio on families deeper than 32767 reads (e.g. measuredaD 59034 vs 32767). The three callers were inconsistent with each other and with fgbio, and this had been re-litigated repeatedly (the per-base arrays were hardened before, but the scalars were left uncapped).How
clamp_per_base_to_fgbio_short(u16) -> i32helper.max/min/sum. For the combined duplex/codeccD/cM, cap each strand's per-base value before summing, exactly as fgbio'stotalDepths(i) = min(abD_i, 32767) + min(baD_i, 32767)— so a mixed pair where only one strand saturates yields the correct intermediate value (e.g. 62767), not an uncapped sum.aE/bE/cEare now summed from per-base errors capped at the ceiling, matching fgbio's cappederrorsArray[Short]. Both the numerator and the denominator of every error rate now derive from Short-capped per-base values.--min-readsthreshold, so no behavior is lost.Tests
u16::MAX).cDcase (aD→32767,bD→30000,cD→62767), alongside the existing vanilla saturation test.aE= 32767/(32767·4) = 0.25) rather than the uncapped value (0.3052); verified to fail before the numerator fix.Notes
!): changes the emitted depth- and error-rate-tag values for consensus families deeper than 32767 reads (now capped, matching fgbio).nh/compare-hardening(feat(compare): harden fgumi compare into a sound, faithful fgbio-parity oracle #530), which drops the now-unnecessary depth-saturation tolerance and compares these tags exactly. With the error numerators now capped too, the tolerance can be dropped foraE/bE/cEas well, not just the depth tags.Summary by CodeRabbit
Bug Fixes
Tests