fix(codec,consensus): soft-only overlap-clip boundary, MC-independent codec clip, fgbio-faithful quality masking (CODEC3-03/04/05/06) - #533
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 selected for processing (4)
WalkthroughMate-based clipping now derives soft-only boundaries from MC tags or mate records, while consensus quality masking recognizes lowercase padding and gives single-strand masking precedence over outer masking. Unit and integration tests cover the revised clipping and masking behavior. ChangesCODEC overlap and masking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Codec
participant Overlap
participant QualityMasking
participant ConsensusBAM
Codec->>Overlap: compute symmetric mate-extension clips
Overlap-->>Codec: return R1/R2 clip lengths
Codec->>QualityMasking: mask outer and single-strand qualities
QualityMasking-->>Codec: return masked consensus qualities
Codec->>ConsensusBAM: write consensus record
🚥 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 #533 +/- ##
==========================================
+ Coverage 92.94% 92.95% +0.01%
==========================================
Files 167 167
Lines 103119 103337 +218
==========================================
+ Hits 95845 96059 +214
- Misses 7274 7278 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2e793c2 to
9598288
Compare
|
Both reviewers run (§0 step 6); all findings were in-diff and addressed by amending the commit. CodeRabbit CLI (
Local CR-style review — 1 finding, fixed: the public rustdoc on
|
|
Parity note: the before/after evidence was additionally re-confirmed against the released fgbio 4.1.0 ( |
9598288 to
4a05caf
Compare
4a05caf to
7289c94
Compare
7289c94 to
668c0cc
Compare
668c0cc to
e8ee6b0
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
2259d6b to
9adc759
Compare
9adc759 to
814acb0
Compare
|
@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 3405-3425: Update
test_mask_consensus_quals_outer_first_then_single_strand_wins so an outer-only
position starts below Q10, such as Q5, while retaining the overlap position’s
behavior. Adjust the expected masked qualities to require that outer masking
assigns Q10 to that position, thereby distinguishing assignment from min while
preserving single-strand Q20 precedence at position 0.
In `@crates/fgumi-raw-bam/src/overlap.rs`:
- Around line 206-207: Update the alignment-end calculation in the
overlap-processing function around reference_length_from_cigar and alignment_end
to use a checked or saturating addition that cannot overflow when ref_len is
oversized. Ensure malformed raw-BAM input fails closed before clipping, and add
a regression test covering an oversized reference-consuming CIGAR.
🪄 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: cc168976-61e9-42e0-8e18-4056e16bf68e
📒 Files selected for processing (4)
crates/fgumi-consensus/src/codec_caller.rscrates/fgumi-raw-bam/src/lib.rscrates/fgumi-raw-bam/src/overlap.rstests/integration/test_codec_command.rs
… codec clip, fgbio-faithful quality masking
Codec masking/clip parity fixes (CODEC3-03/04/05/06); CODEC3-07 documented
as an intentional divergence.
- CODEC3-03: the consensus overlap-clip mate boundary
(num_bases_extending_past_mate_raw, shared by the simplex/duplex/codec
callers) counted soft+hard clips; fgbio uses soft-only
(mateUnSoftClippedStart/End). Compute it soft-only. The samtools-style
soft+hard unclipped 5' used for template-coordinate sort/group
(mate_unclipped_5prime) is deliberately left unchanged.
- CODEC3-04: codec derives the overlap-clip boundary from the mate record in
hand (num_bases_extending_past_mate_vs_mate_raw) rather than the read's MC
tag, so clipping still occurs when MC is absent -- mirroring fgbio
updateMateCigars, which backfills the mate CIGAR from the in-group mate.
- CODEC3-05: single-strand quality masking missed the lowercase-'n'-padded
single-strand tails (it checked uppercase 'N' only); now matches fgbio's
a(i) in {n,N} || b(i) in {n,N}.
- CODEC3-06: outer-base masking now runs first and assigns (not min),
matching fgbio maskCodecConsensusQuals order and semantics.
CODEC3-07: fgumi groups reads by the composite key MI\tCB, so a
within-molecule cell-barcode conflict cannot occur and fgbio's require(<=1)
guard is inapplicable -- an intentional single-cell divergence, documented in
code (not a fix).
Verified against fgbio 4.x CallCodecConsensusReads: before-fix outputs
diverged (unmasked tails at Q45 vs Q20; hard-clip and MC-absent molecules
rejected by fgumi but consensus-called by fgbio); after-fix BAM content
(seq+cigar+qual) is byte-identical across all fixtures.
814acb0 to
6048732
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
…#760) RawRecordClipper::clip_extending_past_mate_ends (the live 'fgumi clip' path) measured read-through past the mate in reference space, so an indel near the clip boundary over-clipped (whole-read clip -> the read unmapped, and its mate then left unclipped) or under-clipped. It now delegates the count to the query-space core (num_bases_extending_past_mate_vs_mate_raw), computes both reads' counts before clipping either (order-independence), and adds existing 3' hard-clipping back into the total. Ports fgbio#1172's deterministic cases and adds property tests (never-unmap, idempotent, symmetric). Removes the now-redundant reference-space duplicates: SamRecordClipper's past-mate methods (that struct is never constructed in production) and record_utils::num_bases_extending_past_mate; redirects the #[cfg(test)] consensus bridge to the correct core. The FR gate goes through the symmetric is_primary_fr_pair_raw (via the vs_mate entry) rather than per-record is_fr_pair_raw, adopting fgumi's established classification (#505/#533) that is intentionally more correct than fgbio/htsjdk on dovetails. The general simplex/duplex consensus path still uses the per-record gate (see #839).
…#760) RawRecordClipper::clip_extending_past_mate_ends (the live 'fgumi clip' path) measured read-through past the mate in reference space, so an indel near the clip boundary over-clipped (whole-read clip -> the read unmapped, and its mate then left unclipped) or under-clipped. It now delegates the count to the query-space core (num_bases_extending_past_mate_vs_mate_raw), computes both reads' counts before clipping either (order-independence), and adds existing 3' hard-clipping back into the total. Ports fgbio#1172's deterministic cases and adds property tests (never-unmap, idempotent, symmetric) over a generator that exercises real read-through (dovetail) and aligned-base overhang clips, with an in-code non-vacuous-coverage check. Removes the now-redundant reference-space duplicates: SamRecordClipper's past-mate methods (that struct is never constructed in production) and record_utils::num_bases_extending_past_mate; redirects the #[cfg(test)] consensus bridge to the correct core. The FR gate goes through the symmetric is_primary_fr_pair_raw (via the vs_mate entry) rather than per-record is_fr_pair_raw, adopting fgumi's established classification (#505/#533) that is intentionally more correct than fgbio/htsjdk on dovetails. The general simplex/duplex consensus path still uses the per-record gate (see #839).
…#760) RawRecordClipper::clip_extending_past_mate_ends (the live 'fgumi clip' path) measured read-through past the mate in reference space, so an indel near the clip boundary over-clipped (whole-read clip -> the read unmapped, and its mate then left unclipped) or under-clipped. It now delegates the count to the query-space core (num_bases_extending_past_mate_vs_mate_raw), computes both reads' counts before clipping either (order-independence), and adds existing 3' hard-clipping back into the total. Ports fgbio#1172's deterministic cases and adds property tests (never-unmap, idempotent, symmetric) over a generator that exercises real read-through (dovetail) and aligned-base overhang clips, with an in-code non-vacuous-coverage check. Removes the now-redundant reference-space duplicates: SamRecordClipper's past-mate methods (that struct is never constructed in production) and record_utils::num_bases_extending_past_mate; redirects the #[cfg(test)] consensus bridge to the correct core. The FR gate goes through the symmetric is_primary_fr_pair_raw (via the vs_mate entry) rather than per-record is_fr_pair_raw, adopting fgumi's established classification (#505/#533) that is intentionally more correct than fgbio/htsjdk on dovetails. The general simplex/duplex consensus path still uses the per-record gate (see #839).
This branch was previously deployed
1 participant
Summary
W6 of the final-audit burn-down: the codec masking/clip cluster. Drives CODEC3-03/04/05/06 to fgbio parity and documents CODEC3-07 as an intentional divergence. Stacked on #505 (
nh/fix-codec-fr-pair-rejection) — the only open PR touchingoverlap.rs/codec_caller.rs— and retargets tomainon merge.Every finding was re-verified against fgbio 4.x source (both sides read), reproduced against the real
fgbio CallCodecConsensusReadsbefore and after, and covered by RED-first unit tests.Findings
CODEC3-03 (S2) — overlap-clip mate boundary counted hard clips
num_bases_extending_past_mate_raw(shared by the simplex, duplex, and codec callers, mirroring fgbioUmiConsensusCaller.numBasesExtendingPastMate) derived the mate boundary from the MC tag counting soft + hard clips. fgbio usesmateUnSoftClippedStart/End— soft only — so a hard-clipped mate shifted the clip amount. Fixed to compute the boundary soft-only.This is intentionally scoped to overlap clipping. The samtools-style soft + hard unclipped 5' position used for template-coordinate sorting and grouping (
mate_unclipped_5prime, consumed byfgumi-sortand the grouping pipeline) is deliberately left unchanged — that key must follow samtools' convention, not fgbio's.CODEC3-04 (S3) — overlap clip silently skipped when MC absent
The MC-tag path returns
0(no clip) when the MC tag is missing, silently under-clipping. Codec always holds both primary FR reads, so it now derives the boundary from the mate record in hand (num_bases_extending_past_mate_vs_mate_raw), mirroring fgbioupdateMateCigars(which backfills the mate CIGAR from the in-group mate before clipping). The MC-based entry point is retained for the simplex/duplex callers.CODEC3-05 (S2 → S1 with
--single-strand-qual) — single-strand tails never maskedpad_consensusfills single-strand tails with lowercasen(NO_CALL_BASE_LOWER), but the mask check tested only== NO_CALL_BASE(uppercaseN), so the tails — the exact target of--single-strand-qual— were never masked. Now matches fgbio'sa(i) ∈ {n,N} || b(i) ∈ {n,N}.CODEC3-06 (S2, conditional) — outer-base masking order and semantics
fgbio applies outer masking first and assigns the quality; fgumi applied single-strand first, then outer as
min. Rewrotemask_consensus_quals_query_basedto match fgbiomaskCodecConsensusQuals: outer first (assign), single-strand second (assign) — so single-strand masking wins where the two regions overlap.CODEC3-07 (S3) — cell-barcode conflict guard: intentional divergence (documented, not fixed)
fgbio groups by MI only, then
requires a single distinct CB across the molecule and errors on conflict. fgumi groups by the composite keyMI\tCB(MiGroupIterator::with_cell_tag, wired incodec.rs), so every record reaching a single consensus group already shares one cell barcode — fgbio's cross-barcode conflict guard is structurally inapplicable. This is an intentional single-cell divergence (consistent with fgumi's opinionated hardcoded-CB design); documented in code, no behavior change.Real-tool parity evidence (fgbio 4.x
CallCodecConsensusReads)Fixtures: a single-strand-tail molecule (
--single-strand-qual 20 --outer-bases-qual 10), a hard-clipped-mate molecule, and an MC-absent molecule.minat the ends; it rejected the hard-clip and MC-absent molecules that fgbio consensus-called.Tests
overlap.rs: soft-only mate boundary (..._soft_only_mate_end), MC-independent mate-record path (..._vs_mate_raw_no_mc_uses_mate), and MC/mate-record agreement without hard clips. RED-verified (soft+hard gives 5, soft-only gives 10).codec_caller.rs:mask_consensus_quals_query_basedlowercase-nmasking (CODEC3-05) and outer-first-then-single-strand-wins (CODEC3-06). Both RED-verified against the old logic.test_codec_command.rs: end-to-end--single-strand-qualmasking of the single-strand tails (baseline vs masked), addressing the BS5 "option-gated behavior never exercised" gap.cargo ci-fmt && cargo ci-lint && cargo ci-testall clean (2216 tests).Not in this PR
rejection.rs.Summary by CodeRabbit
ntails and to apply outer masking first, with single-strand masking overriding on overlaps.--single-strand-qual(baseline vs masked tail qualities) and expanded overlap/masking regression coverage.