Repository navigation
fix(group): reject UMIs of differing length in all assigners (GRP-01) - #510
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:
WalkthroughEncodable UMIs with mixed base lengths now fail before clustering; invalid UMIs are excluded from length checks and receive stable per-string molecule assignments. Sequential and parallel paths share this behavior, with regression coverage for paired UMIs and truncation. ChangesUMI assignment consistency
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
❌ Your patch check has failed because the patch coverage (85.41%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #510 +/- ##
==========================================
+ Coverage 91.12% 92.59% +1.47%
==========================================
Files 78 166 +88
Lines 51540 100040 +48500
==========================================
+ Hits 46964 92629 +45665
- Misses 4576 7411 +2835 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
265b330 to
3745dc7
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@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: 1
🤖 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-umi/src/assigner.rs`:
- Around line 989-991: The sequential and parallel assignment paths validate
different UMI sets before assert_uniform_umi_length. Update the logic around
assert_uniform_umi_length at crates/fgumi-umi/src/assigner.rs:989-991 and
1884-1888 so invalid, non-BitEnc-encodable UMIs are filtered before length
validation in both paths, or consistently rejected before filtering; ensure
mixed valid/invalid inputs produce identical behavior across sequential,
parallel edit, and paired paths.
🪄 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: bc7769ae-4c15-4072-b2b1-d3f27e7a4a69
📒 Files selected for processing (3)
crates/fgumi-umi/src/assigner.rssrc/lib/commands/group.rssrc/lib/umi/parallel_assigner.rs
3745dc7 to
718460d
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-umi/src/assigner.rs`:
- Around line 651-688: Make the production-facing base_umi_length function
private, or move its logic into the tests that need it; do not expose it
publicly when there are no production call sites. Keep assert_uniform_umi_length
public and unchanged.
🪄 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: 03412e25-a8db-4ca8-ac7f-395cd31fad96
📒 Files selected for processing (3)
crates/fgumi-umi/src/assigner.rssrc/lib/commands/group.rssrc/lib/umi/parallel_assigner.rs
718460d to
032beae
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
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-umi/src/assigner.rs`:
- Around line 2105-2155: Extend
test_sequential_assigner_excludes_invalid_umis_from_length_guard with an
fgbio-derived expected baseline for the mixed valid/invalid Edit and Paired
inputs, and assert fgumi’s panic/grouping behavior matches it. If fgbio
intentionally differs for these cases, document that divergence explicitly in
the test instead of leaving the expected behavior unsupported.
In `@src/lib/umi/parallel_assigner.rs`:
- Around line 1068-1120: Extend the regression tests around
test_parallel_assigner_rejects_differing_umi_lengths and
test_parallel_assigner_excludes_invalid_umis_from_length_guard to
programmatically obtain the corresponding fgbio baseline results, then assert
each parallel assigner matches that baseline. Preserve the existing
differing-length panic expectation and mixed valid/invalid grouping assertions
while adding coverage for both the sequential/local behavior and parallel
parity.
🪄 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: a7d99c93-839d-48e6-b075-386855874979
📒 Files selected for processing (3)
crates/fgumi-umi/src/assigner.rssrc/lib/commands/group.rssrc/lib/umi/parallel_assigner.rs
6396833 to
bd1f81c
Compare
|
Addressed the two length-guard baseline comments ( Traced to fgbio source: for the mixed valid/invalid input, fgbio never drops non-encodable UMIs, so |
|
@coderabbitai review |
✅ Action performedReview finished.
|
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-umi/src/assigner.rs`:
- Around line 980-988: Update the sequential edit-assignment flow around
assert_uniform_umi_length to skip non-encodable UMIs during matching, preventing
duplicate invalid values from being deduplicated into one molecule. Allocate
each invalid input its own molecule ID while constructing results, matching
parallel assignment output; add a parity fixture covering duplicate invalid UMIs
such as ["AAAA", "NNN", "NNN"].
- Around line 2132-2165: Extend
test_sequential_assigner_excludes_invalid_umis_from_length_guard with an
Adjacency case using mixed valid and invalid UMIs, then update the sequential
adjacency fast path so invalid UMIs receive separate assignments rather than
sharing the sole encoded UMI’s ID. Add coverage for empty and single-read
families, and verify sequential and parallel adjacency outputs are identical
while preserving valid-UMI grouping.
🪄 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: 78bd6a26-1ced-424e-b74e-fa6df1e3d9b0
📒 Files selected for processing (3)
crates/fgumi-umi/src/assigner.rssrc/lib/commands/group.rssrc/lib/umi/parallel_assigner.rs
bd1f81c to
d03a568
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fgumi-umi/src/assigner.rs (1)
1938-1968: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winExclude non-
BitEncpaired UMIs before counting/grouping. Invalid canonical strings can still hit theumi_counts.len() == 1fast path and getPairedA/B, and same-length invalid neighbors can be pulled into the graph and join a valid molecule. Filterumi_countsto encodable entries up front, then route the dropped UMIs through theSinglefallback.🤖 Prompt for 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. In `@crates/fgumi-umi/src/assigner.rs` around lines 1938 - 1968, The paired assignment flow must exclude non-BitEnc UMIs before fast-path and adjacency grouping. Filter `umi_counts` to BitEnc-encodable entries up front, use the filtered counts for `umi_counts.len()` and `build_adjacency_graph`, and route dropped invalid UMIs through the `Single` fallback while preserving valid paired assignment behavior.
🤖 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.
Outside diff comments:
In `@crates/fgumi-umi/src/assigner.rs`:
- Around line 1938-1968: The paired assignment flow must exclude non-BitEnc UMIs
before fast-path and adjacency grouping. Filter `umi_counts` to BitEnc-encodable
entries up front, use the filtered counts for `umi_counts.len()` and
`build_adjacency_graph`, and route dropped invalid UMIs through the `Single`
fallback while preserving valid paired assignment behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 328ccc45-ee8a-4532-96b0-7834f77c5d34
📒 Files selected for processing (3)
crates/fgumi-umi/src/assigner.rssrc/lib/commands/group.rssrc/lib/umi/parallel_assigner.rs
fgbio's GroupReadsByUmi throws on UMIs of differing length: its adjacency
assigner has an explicit require(orderedNodes.forall(_.umi.length == umiLength),
"Multiple UMI lengths"), and Sequences.countMismatches requires equal lengths
(GroupReadsByUmiTest.scala:513, "fail when umis have different length").
fgumi's count_mismatches instead returns usize::MAX for differing lengths, which
every mismatch-based matcher reads as "no match", so a differing-length UMI
silently forms its own molecule -- under-grouping with no error. Only the
sequential adjacency assigner carried the guard (an inline assert!); the
sequential paired/edit assigners and all three parallel assigners
(edit/adjacency/paired) did not, so the default threaded path under-grouped
silently for every strategy.
Add a shared assert_uniform_umi_length helper in fgumi-umi::assigner and wire it
into every mismatch-based assigner path, replacing the ad-hoc adjacency check.
Identity grouping is exact-match and needs no guard, matching fgbio's identity
assigner. --min-umi-length still works: it truncates every UMI to a common
length before assignment (covered by a new truncate->assign test).
Also make the sequential and parallel assigners agree on invalid
(non-BitEnc-encodable) UMIs, which are production-reachable: validate_umi (like
fgbio, GroupReadsByUmi.scala:673) filters only uppercase-N UMIs upstream, so a
UMI with a lowercase 'n' or a non-ACGT IUPAC base passes the filter and reaches
assign() as non-encodable. Previously the six assigner paths handled these
inconsistently: sequential edit string-matched them (grouping N-neighbours),
parallel edit/adjacency/paired split duplicate invalid UMIs into distinct
molecules, and the sequential adjacency single-UMI fast path tagged an invalid
UMI into the real molecule. Depending on --threads, identical reads grouped
differently.
Unify all six paths on fgbio's map-by-string model: every invalid UMI gets its
own molecule keyed by its uppercased string (identical strings share, distinct
strings never merge, an invalid UMI never joins a valid molecule), via a shared
assign_with_invalid_fallback helper. The one residual divergence from fgbio is
structural: fgbio treats N as a mismatch character and can merge a
within-threshold N-neighbour, but BitEnc cannot represent N, so fgumi isolates
such a UMI; both fgumi assigners agree on the isolation.
Add test_sequential_and_parallel_assigners_induce_same_partition, a
cross-assigner parity harness covering valid grouping plus the invalid-UMI edge
cases per strategy, and run the invalid-UMI behavioural tests on both the
sequential and parallel assigners.
The paired sequential path needed one extra step to actually match that model: the
group command tags each half of a paired UMI with an orientation prefix
(lower_read_umi_prefix/higher_read_umi_prefix, e.g. "aa:ACGT-bb:TTTT") before calling
assign(), so a whole-key BitEnc check reads every production key as non-encodable -- it
would isolate every molecule as a filter and never fire as the length guard. The paired
assigner now strips that prefix per half (underlying_umi_len) before deciding
encodability, so genuinely non-encodable paired UMIs (e.g. a lowercase-n / IUPAC base
that bypassed the uppercase-N filter) are isolated to their own molecule -- matching the
parallel paired assigner and grouping the same reads identically regardless of --threads
-- while the differing-length guard now fires on the prefixed production keys instead of
silently no-op'ing.
Parity (Strategy Paired/Adjacency/Edit, edits=1, RX "ACT-ACT" vs "ACT-AC"):
before: fgbio rejects (exit 1); fgumi Paired/Edit exit 0 (silent under-group),
Adjacency exit 101
after: fgbio rejects (exit 1); fgumi rejects all three (exit 101)
Tracker: GRP-01 (S3/S1).
d03a568 to
8b6838b
Compare
|
Addressed the outside-diff comment on the paired assigner ( The literal suggestion — filter Fixed it prefix-aware instead: a new Coverage: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/umi/parallel_assigner.rs (2)
686-711: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftParse prefixed paired keys before
BitEncencoding.Production keys such as
aa:ACGT-bb:TTTTcontain:and aBprefix, so whole-string encoding filters valid UMIs out. They then hit the all-invalid branch, lose/A//B, and bypass both adjacency and length validation. Share the sequential prefix-aware parsing and add a parallel production-key regression.As per path instructions, parallel paired assignment must match the sequential path and fgbio baseline.
🤖 Prompt for 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. In `@src/lib/umi/parallel_assigner.rs` around lines 686 - 711, Update the parallel paired-assignment preprocessing around sorted_umis to parse production-prefixed paired keys with the same prefix-aware logic used by the sequential assigner before calling BitEnc::from_umi_str. Preserve the original key for counts and output while encoding the parsed UMI components, so /A and /B handling, adjacency assignment, and uniform-length validation remain active; add a regression covering prefixed production keys.Source: Path instructions
770-792: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKey invalid paired fallbacks by the raw uppercase string.
Canonical keys merge distinct reversed invalid strings only when a valid UMI is also present. For example,
ACGN-TTTTandTTTT-ACGNshare an ID here, but remain distinct in the sequential and all-invalid paths.Proposed fix
} else { - *invalid_to_id.entry(canonical).or_insert_with(|| { + *invalid_to_id.entry(umi.to_uppercase()).or_insert_with(|| { let id = MoleculeId::Single(next_mol_id); next_mol_id += 1; idAdd this reversed-invalid case to the parity harness.
As per path instructions, parallel assignment must produce output identical to sequential assigners.
🤖 Prompt for 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. In `@src/lib/umi/parallel_assigner.rs` around lines 770 - 792, Update the invalid fallback in the raw_umis mapping to key invalid UMIs by their raw uppercase string rather than the canonicalized value, while retaining canonical_to_mol lookup and strand assignment for valid UMIs. Ensure reversed invalid UMIs receive distinct MoleculeId::Single values and match sequential/all-invalid behavior. Extend the parity harness with the reversed-invalid case such as ACGN-TTTT and TTTT-ACGN.Source: Path instructions
🤖 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.
Outside diff comments:
In `@src/lib/umi/parallel_assigner.rs`:
- Around line 686-711: Update the parallel paired-assignment preprocessing
around sorted_umis to parse production-prefixed paired keys with the same
prefix-aware logic used by the sequential assigner before calling
BitEnc::from_umi_str. Preserve the original key for counts and output while
encoding the parsed UMI components, so /A and /B handling, adjacency assignment,
and uniform-length validation remain active; add a regression covering prefixed
production keys.
- Around line 770-792: Update the invalid fallback in the raw_umis mapping to
key invalid UMIs by their raw uppercase string rather than the canonicalized
value, while retaining canonical_to_mol lookup and strand assignment for valid
UMIs. Ensure reversed invalid UMIs receive distinct MoleculeId::Single values
and match sequential/all-invalid behavior. Extend the parity harness with the
reversed-invalid case such as ACGN-TTTT and TTTT-ACGN.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dd5b6bd5-139b-4240-a533-e162963ad85f
📒 Files selected for processing (3)
crates/fgumi-umi/src/assigner.rssrc/lib/commands/group.rssrc/lib/umi/parallel_assigner.rs
|
Re the two outside-diff findings on 1. "Parse prefixed paired keys before 2. "Key invalid paired fallbacks by the raw uppercase string" (770-792) — real, fixed in the stacked #525. Confirmed: the parallel main-path fallback keyed invalid UMIs by their canonical form, so reverses like The fix ( |
The sequential edit, adjacency, and paired assigners each re-ran BitEnc::from_umi_str several times per UMI and discarded the result, keeping only .len() or .is_some(). On the paired path this compounded: the differing-length guard added in #510 called underlying_umi_len (which encodes both halves of the prefixed key) up to three times per UMI -- in the encodability filter, the length guard, and the per-record strand resolution in the single-molecule fast path. Encode each UMI once into a Vec<Option<BitEnc>> (paired: a Vec of underlying base lengths) and reuse it for all three sites. BitEnc is a 16-byte Copy struct, so caching it is far cheaper than repeating the per-byte encode. count_paired now carries the underlying length; this is sound because canonicalization only swaps the two halves (A-B <-> B-A), which preserves both BitEnc-encodability and the summed base count, so every raw UMI folding into a canonical form contributes the same value. Grouping output is unchanged: family-size histograms and molecule counts are byte-identical before and after. On a c7g.4xlarge at threads=0 this recovers ~1% of paired group runtime on agilent-hs2 (93.4s -> 92.4s user), with the untouched identity strategy flat as a control. Encode calls per UMI: edit 2->1, adjacency 2->1, paired 6->2.
The sequential edit, adjacency, and paired assigners each re-ran BitEnc::from_umi_str several times per UMI and discarded the result, keeping only .len() or .is_some(). On the paired path this compounded: the differing-length guard added in #510 called underlying_umi_len (which encodes both halves of the prefixed key) up to three times per UMI -- in the encodability filter, the length guard, and the per-record strand resolution in the single-molecule fast path. Encode each UMI once into a Vec<Option<BitEnc>> (paired: a Vec of underlying base lengths) and reuse it for all three sites. BitEnc is a 16-byte Copy struct, so caching it is far cheaper than repeating the per-byte encode. count_paired now carries the underlying length; this is sound because canonicalization only swaps the two halves (A-B <-> B-A), which preserves both BitEnc-encodability and the summed base count, so every raw UMI folding into a canonical form contributes the same value. Grouping output is unchanged: family-size histograms and molecule counts are byte-identical before and after. On a c7g.4xlarge at threads=0 this recovers ~1% of paired group runtime on agilent-hs2 (93.4s -> 92.4s user), with the untouched identity strategy flat as a control. Encode calls per UMI: edit 2->1, adjacency 2->1, paired 6->2.
This branch was previously deployed
1 participant
What
fgbio's
GroupReadsByUmithrows on UMIs of differing length: its adjacency assigner has an explicitrequire(orderedNodes.forall(_.umi.length == umiLength), "Multiple UMI lengths"), andSequences.countMismatchesrequires equal lengths (GroupReadsByUmiTest.scala:513, "fail when umis have different length").fgumi's
count_mismatchesinstead returnsusize::MAXfor differing lengths, which every mismatch-based matcher reads as "no match", so a differing-length UMI silently forms its own molecule — under-grouping with no error. Only the sequential adjacency assigner carried the guard (an inlineassert!); the sequential paired/edit assigners and all three parallel assigners (edit/adjacency/paired) did not, so the default threaded path under-grouped silently for every strategy.Change
assert_uniform_umi_lengthhelper (plusbase_umi_length) infgumi-umi::assigner, and wire it into every mismatch-based assigner path — sequential edit/adjacency/paired and parallel edit/adjacency/paired — replacing the ad-hoc inline adjacency check.Identitygrouping is exact-match and needs no length guard, matching fgbio's identity assigner.--min-umi-lengthstill works: it truncates every UMI to a common length before assignment, so the guard never fires on the variable-length input the flag supports. A newtruncate → assigntest (test_variable_length_umis_group_after_truncation) locks that interaction end-to-end.Parity evidence (§0 step 2/4)
Fixture:
reports/fgbio-parity-fixtures/grp-01-unequal-umi-length.sh— two read pairs at the same coordinates/orientation,RX=ACT-ACT(7 chars) vsACT-AC(6 chars), run underStrategy.Paired/Adjacency/Edit,edits=1(mirrorsGroupReadsByUmiTest.scala:513).Both tools now reject differing-length UMIs (non-zero exit) across all three strategies; before, Paired and Edit silently under-grouped.
Notes
assert!), consistent with the existing sequential-adjacency guard and the sibling"is not a paired UMI"guards in the same file. Exit is non-zero (101) rather than fgbio's 1, which satisfies the missing-guard (S3/BS6) parity standard used throughout the tracker; converting theassigntrait to returnResultwould be an invasive refactor rippling intodedupand is out of scope for this parity fix.Tests
should_paniccases for all six assigner paths (sequential + parallel × edit/adjacency/paired) asserting"Multiple UMI lengths", plus direct unit tests forassert_uniform_umi_length/base_umi_length, and the--min-umi-lengthregression test.cargo ci-fmt && cargo ci-lint && cargo ci-testall green (2219 passed).Tracker: GRP-01 (S3/S1). GRP-02 wontfix.
Summary by CodeRabbit
assert_uniform_umi_length(...)to enforce consistent UMI lengths, including consistent paired behavior (delimiter ignored).