Repository navigation
test(consensus): pin read-order independence of alignment grouping - #1037
Conversation
The fgbio audit flagged the alignment-group CIGAR comparator as order-dependent: with input sorted by length descending, a prefix CIGAR was said to be absorbed into the longer CIGAR's group. This does not reproduce. Grouping runs over a stable descending-length sort of the reads, as fgbio's filterToMostCommonAlignment does (sortBy -length). A prefix CIGAR is never longer than the CIGARs it prefixes, and it is added to every matching group without breaking early. The comparator only breaks size ties between groups, and it is a total order over distinct CIGARs, so it cannot depend on input order either. One case is order-dependent in both tools and is not pinned: a prefix of equal query length (its extension consumes no query, e.g. 25M vs 25M1D) ties in the stable sort, so input order decides which read seeds the group. Hard-clipped records can also group differently from fgbio, because fgumi simplifies the CIGAR before truncating it; that is out of scope here. Add regression tests: - ports of fgbio's four tie-breaking tests (VanillaUmiConsensusCallerTest.scala:583, :608, :630, :652), which fgumi did not have - equal-query-length tie cases whose input orders create the groups in opposite orders, including one where element length and operator disagree, pinning length-before-operator priority - a fixed-order 25M vs 25M1D case pinning the element-count tie-break - six prefix-related reads in all 720 input orders, asserting the same kept and rejected read ids, with kept reads in input order - an end-to-end consensus over three of those orders, asserting the consensus base that identifies the winning group, the per-base and maximum depth, and byte-identical output Move itertools to the workspace dependencies so fgumi-consensus can use its permutations in tests. No output change.
|
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 configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. 3 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 4 reviews per hour.
WalkthroughNo production behavior change is shown. The change centralizes the ChangesCIGAR Selection Tests
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Merge Risk: ⚪ Minimal · up to The added tests and dependency declarations present no identified obstacle to merging after normal checks. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1037 +/- ##
=======================================
Coverage 96.47% 96.48%
=======================================
Files 299 299
Lines 152214 152288 +74
=======================================
+ Hits 146854 146928 +74
Misses 5360 5360 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was successfully deployed
1 participant
Summary
The fgbio parity audit reported that the alignment-group CIGAR comparator (
cmp_cigarinselect_most_common_alignment_group) depends on input order: when the reads arrive longest-first, a prefix CIGAR was said to be absorbed into the longer CIGAR's group. That does not reproduce, so this PR adds tests only and changes no production code.Grouping does not depend on input order:
filter_source_reads_by_alignment(and the CODEC caller) sort reads by descending query length with a stable sort before grouping, as fgbio'sfilterToMostCommonAlignmentdoes withsortBy(-length).is_cigar_prefixmatches fgbio'sCigar.isPrefixOf.cmp_cigarorders element by element (length, then operator: M < I < D < N < S < H < P < = < X), then by element count. That is fgbio'sCigar.cigarOrdering, andmax_by(size).then(cmp_cigar(b, a))matchesmaxBy((size, cigar))(Ordering.Int, cigarOrdering.reverse). Group CIGARs are distinct, so this is a total order with no ties, and the winning group cannot depend on input order.No output change.
fgbio parity (e51a661)
UmiConsensusCaller.scala:406-436(filterToMostCommonAlignment): stable length sort, no-break prefix grouping, and the size-then-smallest-CIGAR tie-break that fgumi implements.Alignment.scala:57,102-112(cigarElemOrdering,cigarOrdering): length before operator, then element count.VanillaUmiConsensusCallerTest.scala:583,:608,:630and:652.One quirk is shared with fgbio and not pinned: reads with the same query length whose CIGARs are prefix-related (e.g.
25Mand25M1D) can group differently depending on input order, because the first such read seeds the group.Out of scope: for hard-clipped records, fgumi simplifies the CIGAR before truncating it and fgbio truncates first, so the two tools can group such reads differently. That divergence is fixed in a separate PR.
Tests
All in
crates/fgumi-consensus/src/vanilla_caller.rs, under the existingfilterToMostCommonAlignmentsection:test_filter_tied_alignment_groups_select_same_winner_in_any_input_order: the four fgbio ports, plus three cases whose tied groups share a query length, so their orderings create the groups in opposite orders: smaller first element,IbeforeD, and element length before operator (25M1D25Mbeats25M2I23M).test_filter_tied_prefix_groups_prefer_fewer_elements:[25M, 25M1D]keeps25M, pinning the element-count tie-break (fgbio's:630port is decided by the first element instead).test_filter_prefix_cigars_group_identically_in_any_input_order: six prefix-related reads in all 720 input orders (viaitertools::permutations), asserting the same kept and rejected ids every time.test_consensus_with_prefix_cigars_is_identical_in_any_input_order: end to end, asserting the consensus base that identifies the winning group, the per-base and max depth, and byte-identical output across orders.itertoolsmoves to[workspace.dependencies], since it is now used by two members, and is added as a dev-dependency offgumi-consensus.Mutation-checked: each of these fails at least one new test: flipping or removing the element-count tie-break, comparing operator before length, reversing the element-length or operator order, swapping the
I/Dordinal, dropping or reversing the CIGAR tie-break, removing or reversing the length sort, and stopping at the first matching group.Checks
cargo ci-fmt,cargo ci-lint,cargo ci-doc,cargo ci-doctest,cargo ci-tag-literalsandcargo ci-testall pass.