fix(consensus): trim read-through on dovetail FR pairs in the MC-tag path - #853
Conversation
…path `num_bases_extending_past_mate_raw` (used by the simplex and duplex consensus callers to trim read-through past the mate) gated on the per-record `is_fr_pair_raw`, whose forward-strand arm derives the mate's 5' position from TLEN. That misclassifies dovetail FR pairs (htsjdk/samtools#1771): the forward read reads as NOT-FR, the clip returns 0, and its read-through / adapter bases past the mate leak into the consensus. The bug is asymmetric -- the reverse read is unaffected because its branch of `is_fr_pair_raw` is CIGAR-derived. Classify orientation per-pair from the read plus its MC tag instead. A reverse-strand read is the reverse record, so its own CIGAR-based arm is already correct; a forward-strand read's mate is the reverse record, whose leftmost is the mate position and whose alignment end comes from the MC CIGAR, so FR holds iff the read's 5' precedes that end -- the same symmetric branch `is_primary_fr_pair_raw` (CODEC, #505) and `fgumi clip` (#760) use, without needing the mate record in hand. The MC tag is now parsed before the gate; with no usable MC there is no overhang to compute regardless, so it still fails closed to 0. Add a dovetail regression pinning `num_bases_extending_past_mate_raw` on the forward read (50M50S @ 101, TLEN -90, MC 100M; mate 100M @ 61): 40 bases of read-through are now trimmed where the per-record gate returned 0. Closes #839.
|
Note Reviews pausedUse 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 (1)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughThe MC-based overlap path no longer relies on TLEN for forward-read FR classification. It validates MC CIGAR data, handles invalid data by returning zero, and trims 40 read-through bases for forward dovetail pairs. ChangesOverlap FR classification
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized fix trims dovetail read-through correctly in the MC-tag consensus path and includes regression coverage; no actionable merge-blocking risk remains beyond normal checks and review. Suggested labels: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #853 +/- ##
=========================================
Coverage 94.50% 94.50%
=========================================
Files 190 193 +3
Lines 117990 120044 +2054
=========================================
+ Hits 111505 113448 +1943
- Misses 6485 6596 +111 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was successfully deployed
1 participant
Summary
num_bases_extending_past_mate_raw(used by the simplex and duplex consensus callers to trim read-through past the mate) gated on the per-recordis_fr_pair_raw, whose forward-strand arm derives the mate's 5' position fromTLEN. That misclassifies dovetail FR pairs (htsjdk/samtools#1771): the forward read reads as NOT-FR, the clip returns 0, and its read-through / adapter bases past the mate leak into the consensus. The bug is asymmetric — the reverse read is unaffected because its branch ofis_fr_pair_rawis CIGAR-derived.Fix
Classify orientation per-pair from the read plus its
MCtag instead. A reverse-strand read is the reverse record, so its own CIGAR-based arm is already correct; a forward-strand read's mate is the reverse record, whose leftmost is the mate position and whose alignment end comes from theMCCIGAR, so FR holds iff the read's 5' precedes that end — the same symmetric branchis_primary_fr_pair_raw(CODEC, #505) andfgumi clip(#760) use, without needing the mate record in hand. TheMCtag is now parsed before the gate; with no usableMCthere is no overhang to compute regardless, so it still fails closed to 0.Testing
Adds a dovetail regression pinning the forward read (50M50S @ 101,
TLEN-90,MC100M; mate 100M @ 61): 40 bases of read-through are now trimmed where the per-record gate returned 0, and the test cross-checks the MC-tag path against the symmetric mate-record pathnum_bases_extending_past_mate_vs_mate_rawon the same geometry. Full suite, clippy, and fmt pass.Closes #839.
Risk: changes consensus output by correcting 40-base read-through trimming; no
unsafechanges and no CLAUDE.md allowlist update; no memory bound, queue capacity, or thread/backpressure changes.Fixes FR-pair classification in the MC-tag consensus path by using symmetric read-plus-
MCorientation logic. UnusableMCtags fail closed with zero overhang. Regression coverage verifies the affected dovetail geometry and matches the mate-record path.