Repository navigation
fix(group): label paired strands by read orientation in the parallel assigner - #1013
Conversation
…assigner With --strategy paired, group gives each read pair's UMI an orientation prefix (aa: for the half read by the genomically earlier read, bb: for the later one) before UMI assignment. The canonical spelling is then always the one whose R1 is the earlier read, so /A is the strand whose R1 maps first and /B the other strand of the same molecule, as in fgbio. The parallel paired assigner, which group uses under --allow-unmapped or --parallel-group-min-templates, was handed the raw UMI without these prefixes. Two things went wrong: - /A followed the UMI spelling: it was whichever strand's RX sorted first, so about half of the molecules had their strands labelled the other way round from the sequential assigner and from fgbio. - When a molecule's two UMI halves are identical (ACGT-ACGT), both strands have the same raw RX, so they were assigned the same family. That puts reads of both strands into one single-strand consensus. group now sends both paired assigners the same prefixed keys, and the parallel assigner takes the prefixes into account: it 2-bit encodes the prefix-stripped bases, and when the prefixes rule out any reverse-orientation match (they always do for group's prefixes, which are one longer than the edit threshold) it uses forward edges only. Strands are decided on the full prefixed keys, as in the sequential assigner. Any other prefixed pool is grouped on the full strings, which matches the sequential relation exactly. Unprefixed input behaves as before. Behaviour change: on the parallel path, the two strands of an unmapped template pair no longer group together. Unmapped mates have no genomic order, so both strands get the same prefix order and their keys are not reverses of each other. This matches the sequential assigner.
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. 1 included review remains after this review. Your included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. WalkthroughPaired UMI keys now use orientation prefixes in both sequential and parallel assignment. The parallel assigner strips prefixes for encoding and uses full keys and prefixes when selecting comparison paths and assigning strands. Tests compare grouping and strand labels across both assigners. ChangesPaired UMI assignment
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Group as group.rs
participant Assigner as ParallelPairedAssigner
participant Encoder as BitEnc
Group->>Assigner: Orientation-prefixed paired UMI keys
Assigner->>Encoder: Prefix-stripped UMI bases
Encoder-->>Assigner: Encoded UMI bases
Assigner->>Assigner: Select comparison path and assign strands
Suggested labels: Merge Risk: ⚪ Minimal · up to No actionable issue remains identified; the paired-UMI orientation change is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
|
@coderabbitai pause |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1013 +/- ##
==========================================
- Coverage 96.43% 96.42% -0.01%
==========================================
Files 299 299
Lines 151142 151274 +132
==========================================
+ Hits 145748 145870 +122
- Misses 5394 5404 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
✅ Action performedReviews paused. |
This branch was successfully deployed
1 participant
Summary
With
--strategy paired,groupprefixes each half of a read pair's UMI by which read it came from (aa:for the genomically earlier read,bb:for the later one) before assigning molecules. The canonical spelling is then always the one whose R1 is the earlier read, so/Ais that strand and/Bthe other strand of the same molecule. This matches fgbio'sGroupReadsByUmi.The parallel paired assigner, which
groupuses under--allow-unmappedor--parallel-group-min-templates(with--threads > 1), received the raw UMI without those prefixes. That caused two problems:/Awas whichever strand'sRXhappened to sort first, so about half of the molecules got the opposite strand labels from the sequential path and from fgbio.ACGT-ACGT, about 1 in 32 molecules with a fixed 32-UMI set), both strands have the same rawRX. They were put in the same family, so one single-strand consensus mixed reads from both strands.Changes
groupbuilds the same orientation-prefixed key for both paired assigners.ParallelPairedAssignernow exposeslower_read_umi_prefix()/higher_read_umi_prefix(), built exactly likePairedUmiAssigner's.ParallelPairedAssigneraccepts prefixed keys:editspositions, whichgroup's prefixes always do), it keeps theBitEncfast path with forward edges only..coderabbit.yaml: the review instruction that described the old key contract now describes the new one.Behavior changes
--allow-unmapped, the two strands of an unmapped template pair no longer group into one molecule on the parallel path. Unmapped mates have no genomic order, so both strands get the same prefix order. This is what the sequential path already did (fgbio drops unmapped templates before grouping). Before this change the parallel path grouped them (as0/A,0/B) while the sequential path kept them as two molecules.The default path (sequential assigner) is unchanged.
Tests
groupend-to-end:test_paired_strand_follows_read_orientation(sequential and parallel) covers a molecule whose top-strand UMI sorts after its reverse and one with identical halves. The parallel case failed before the fix.groupend-to-end:test_allow_unmapped_paired_parallel_matches_sequentialchecks that--allow-unmappedgives the same molecules and strand labels on both paths. It failed before the fix./A//Blabels: rstest cases (identical halves, halves sorting descending, an adjacency chain, invalidNUMIs, asymmetric halves, mixed case), plus two proptests over random prefixed pools (symmetric and asymmetric halves, edits 1–2, 1/4/16 threads).cargo ci-test,ci-lint,ci-fmt,ci-tag-literalsandci-docpass.groupoutput changes for paired-UMI family assignments and/A//Bstrand labels; sequential-parity, orientation, and unmapped-pair tests pin the intended results. Unsafe: none added, so no CLAUDE.md allowlist update is needed. Memory bounds, queue capacity, and thread/backpressure policy: none changed.The fix gives sequential and parallel paired assigners the same orientation-prefixed keys. The parallel path strips prefixes only for encoding and preserves full keys for comparisons and strand decisions. This prevents unmapped paired strands from merging when the sequential path keeps them separate.