Repository navigation
perf(group): encode each UMI once in the sequential assigners - #623
Conversation
|
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)
WalkthroughInvalid UMIs are encoded once and tracked through validation, matching, canonicalization, and molecule-ID mapping. Simple, adjacency, and paired assigners now apply consistent cached-validity rules. ChangesUMI assignment consistency
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 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 #623 +/- ##
==========================================
+ Coverage 93.52% 93.54% +0.02%
==========================================
Files 175 175
Lines 105404 106015 +611
==========================================
+ Hits 98574 99170 +596
- Misses 6830 6845 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
df0cd7b to
7dde8dc
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 2719-2763: Extend the paired-UMI regression test around
PairedUmiAssigner::count_paired to verify the assignment outcome after invalid
entries with None underlying lengths are filtered, not only the carried lengths.
Add the generated fgbio identity baseline or explicitly assert the documented
invalid-UMI divergence, while preserving the existing canonicalization and
length assertions.
🪄 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: a0fa7da4-7615-4d9d-861f-79fd337e2b74
📒 Files selected for processing (1)
crates/fgumi-umi/src/assigner.rs
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.
7dde8dc to
70f2a73
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was previously deployed
1 participant
Summary
Each sequential UMI assigner (
edit,adjacency,paired) re-ranBitEnc::from_umi_strseveral times per UMI and threw the result away, keeping only.len()or.is_some(). The differing-length guard added in #510 compounded this on the paired path:underlying_umi_len(which splits the prefixed key and encodes both halves) was called 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.This encodes each UMI once and reuses the result everywhere:
from_umi_strcalls per UMI, before → afterunderlying_umi_len)BitEncis a 16-byteCopystruct, so the cachedVec<Option<BitEnc>>(paired: aVec<Option<usize>>of underlying base lengths) is far cheaper than repeating the per-byte encode.Correctness
Behaviour-preserving by construction — every cached value is a pure function of a string that does not change between the cache write and its reads.
count_pairednow carries the underlying base length. This is sound because canonicalization only ever swaps the two halves (A-B↔B-A), which leaves the base multiset — and therefore bothBitEnc-encodability and the summed base count — unchanged, so every raw UMI folding into a canonical form contributes the same value.assign_with_invalid_fallback'sresolveclosure now receives the UMI's position so callers can index the precomputed encoding.test_sequential_and_parallel_assigners_induce_same_partitionpasses unchanged.N-containing UMI stayingNonethrough canonicalization).Full workspace suite: 5589 passed, 23 skipped.
cargo ci-fmt/ci-lintclean.Performance
Measured on a
c7g.4xlarge(Graviton3, matching the benchmark fleet),--threads 0, GNUtime -vuser CPU, 3 reps, with the untouchedidentitystrategy as a control:Complete rep separation on paired (every base rep slower than every branch rep). Family-size histograms and molecule counts are byte-identical before/after (paired 10,220,662; identity 10,292,162).
Scope
This removes the redundant computation on the sequential paths. Profiling attributes the larger share of the paired path's recent cost to per-UMI allocations (
reverse()building a freshStringon every adjacency comparison;canonicalize_paired; the already-uppercase input still being re-allocated). Those are a separate, self-contained follow-up and are intentionally not bundled here.Summary by CodeRabbit
None) through canonicalization and grouping, ensuring invalid paired UMIs remain isolated.Nonepropagation and isolation of invalid inputs.