Repository navigation
perf(group): cut the paired assigner's per-UMI encoding, allocation, and iteration - #625
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)
WalkthroughPaired UMI assignment now performs byte-level BitEnc-compatible validation, avoids unnecessary uppercase and reverse-string allocations, and preserves strand and matching behavior. Tests cover prefixes, invalid bases, length limits, orientations, and mismatch thresholds. ChangesPaired UMI assignment
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 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 #625 +/- ##
==========================================
+ Coverage 93.53% 93.57% +0.03%
==========================================
Files 175 175
Lines 106571 106634 +63
==========================================
+ Hits 99685 99780 +95
+ Misses 6886 6854 -32 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
df0cd7b to
7dde8dc
Compare
b88d273 to
82ee453
Compare
7dde8dc to
70f2a73
Compare
82ee453 to
69d78e4
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
69d78e4 to
e85bca8
Compare
|
@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: 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 2911-2992: Add a programmatically generated fgbio baseline test
for the changed UMI assignment path, comparing assignment partitions and A/B
strand identities rather than only local helper results. Generate cases covering
orientation ties, empty and singleton families, asymmetric halves, and relevant
length boundaries, and assert the implementation matches the fgbio output for
every generated case.
- Around line 1854-1865: Update orientation_is_ab to preserve the full lexical
ordering of the original paired UMI, including accepted prefixes, by comparing
umi with a virtual reverse-order second-first representation rather than
comparing split halves directly. Ensure assign() selects the same A/B strand as
umi < reverse(umi), and add coverage for the valid prefixed case A-A!: where the
comparison must return false.
🪄 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: 40ec937e-f305-44c2-b118-26ccd5f23d8a
📒 Files selected for processing (1)
crates/fgumi-umi/src/assigner.rs
e85bca8 to
34ec4e8
Compare
…and iteration
Follows the encode-once change with the rest of the sequential paired assigner's
per-UMI overhead, identified by bisect as the source of the group paired-strategy
slowdown and confirmed by profiling on a c7g.4xlarge.
- underlying_umi_len: replace split('-')/rsplit(':') iterators plus a double
BitEnc::from_umi_str encode with a single byte scan (position/rposition + an
ACGT count). The packed value was always discarded; only validity and base
length were used. Kept bug-for-bug equivalent to from_umi_str (both cases,
>32-bases-per-half rejection); prefix bytes before ':' are never validated so a
'bb:' prefix is ignored, not rejected.
- Single-molecule fast path: decide the strand with a new allocation-free
orientation_is_ab helper instead of building a reversed String per read.
umi < reverse(umi) reduces to comparing the two dash-halves, since '-' is
smaller than every byte that can appear in a half.
- matches_paired: compare reverse(lhs) to rhs via reversed_matches_within (a
positional byte compare) instead of allocating the reversed string on every
adjacency-graph comparison.
- upper_umis: borrow raw_umis directly when no UMI contains a lowercase byte (the
common case), allocating an uppercased copy only for mixed-case input.
- Paired-format validation: locate the first '-' and confirm no second, instead
of split('-').count().
Grouping output is unchanged: family-size histograms and molecule counts are
byte-identical before and after (agilent-hs2 paired: 10,220,662 molecules). On a
c7g.4xlarge at threads=0 this recovers roughly three-quarters of the paired
regression on agilent-hs2 (from +4% over the pre-regression baseline to ~+1%),
with the untouched identity strategy flat as a control. New rstest tables pin
each rewritten helper to the definition it replaces.
34ec4e8 to
ece62be
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was previously deployed
1 participant
Summary
Stacked on #623. Together with that PR, this removes the per-UMI overhead the sequential
PairedUmiAssignergained — the source of thegrouppaired-strategy slowdown, which a full staircase bisect across v0.3.1→main attributes entirely to a single commit (the differing-length/invalid-UMI GRP-01 correctness fix), with every other commit in the range flat and theidentitycontrol flat throughout.#623 removed the redundant encodes. This removes the rest of the per-UMI work, all in
PairedUmiAssigner:underlying_umi_len— replace thesplit('-')/rsplit(':')iterators plus a doubleBitEnc::from_umi_strencode with a single byte scan (position/rposition+ an ACGT count). The packed value was always discarded — only validity and base length were ever used. Kept bug-for-bug equivalent tofrom_umi_str(both cases accepted, >32-bases-per-half rejected); prefix bytes before:are never validated, so abb:orientation prefix is ignored rather than rejected.orientation_is_abhelper instead of building a reversedStringper read.umi < reverse(umi)reduces to comparing the two dash-halves, because-(0x2D) is smaller than every byte that can appear in a half.matches_paired— comparereverse(lhs)torhsviareversed_matches_within(a positional byte compare) instead of allocating the reversed string on every adjacency-graph comparison.upper_umis— borrowraw_umisdirectly when no UMI contains a lowercase byte (the production case), allocating an uppercased copy only for mixed-case input.-and confirm no second, instead ofsplit('-').count().Correctness
Behaviour-preserving. Grouping output is unchanged: family-size histograms and molecule counts are byte-identical before and after (agilent-hs2 paired: 10,220,662 molecules; identity: 10,292,162). The GRP-01 cross-assigner parity guarantees are untouched (
test_sequential_and_parallel_assigners_induce_same_partitionpasses). Each rewritten helper is pinned to the definition it replaces by a newrstesttable:test_underlying_umi_len_matches_bitenc(10 cases: prefixes, both cases, non-ACGT, the 32-base limit, empty/uneven halves)test_orientation_is_ab_matches_reverse_compare(7 cases vsumi < reverse(umi))test_reversed_matches_within_matches_alloc(8 cases vs reverse-then-compare, incl. threshold boundaries)Full workspace suite: 5614 passed, 23 skipped.
ci-fmt/ci-lintclean.Performance
Measured on a
c7g.4xlarge(Graviton3, matching the benchmark fleet),--threads 0, GNUtime -v, agilent-hs2 paired, withidentityas a flat control. Against the pre-regression baseline (the offending commit's parent):This recovers roughly three-quarters of the paired regression on this workload; the residual ~1% is the irreducible per-UMI validity/length check the GRP-01 fix requires.
Scope note (measured honestly): agilent-hs2 is high-diversity, so its groups take the single-molecule fast path and the adjacency graph is cold. On that workload the
matches_pairedandupper_umischanges are neutral — the recovery above comes from theunderlying_umi_lenand fast-path-strand changes.matches_paired's per-comparison allocation and theupper_umisallocation are nonetheless real; they are expected to help adjacency-heavy / low-diversity inputs (deep panels), which this measurement does not exercise. They are included as correctness-preserving allocation hygiene, not because they move this benchmark.Summary by CodeRabbit
Performance Improvements
Bug Fixes
Tests