Repository navigation
fix(group): emit consecutive MI integers 0..N-1 - #273
Conversation
The `group` and `dedup` commands reserved MI counter blocks sized by template count, but each position group only emits a distinct MI per UMI family. Multiple templates in the same family share a `MoleculeId`, and `PairedA(id)` / `PairedB(id)` share the same numeric id, so the block size was almost always larger than the number of IDs actually used. The unused slots became gaps in the global MI space (observed as ~22% wasted IDs on a 1 M-pair test), which propagated to downstream consensus read names (e.g. `fgumi codec`'s `smoke:N`) and broke cross-tool read-name comparison with fgbio's `GroupReadsByUmi`. Track a `distinct_mi_count` on `ProcessedPositionGroup` and `ProcessedDedupGroup` — computed as `max(MoleculeId::id()) + 1` across assigned templates — and advance the global MI counter by that value instead of `templates.len()`. This applies to both the unified parallel pipeline and the single-threaded streaming path. Adds two integration tests covering both paths that assert `max(MI) == distinct_count - 1`, matching fgbio's output. Fixes #269
📝 WalkthroughWalkthroughThe pull request introduces a 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #273 +/- ##
=======================================
Coverage 89.84% 89.85%
=======================================
Files 120 120
Lines 61359 61379 +20
=======================================
+ Hits 55127 55151 +24
+ Misses 6232 6228 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/commands/group.rs (1)
1287-1297: Extract thedistinct_mi_countcalculation.This
max(id) + 1rule now exists here, again in the single-threaded path below, and insrc/lib/commands/dedup.rs. It’s worth centralizing so the contiguity invariant only has one definition.♻️ Helper sketch
+fn distinct_mi_count(templates: &[Template]) -> u64 { + templates.iter().filter_map(|t| t.mi.id()).max().map_or(0, |max_id| max_id + 1) +} + // ... - let distinct_mi_count: u64 = templates - .iter() - .filter_map(|t| t.mi.id()) - .max() - .map(|max_id| max_id + 1) - .unwrap_or(0); + let distinct_mi_count = distinct_mi_count(&templates);Apply the same helper in
process_and_write_position_groupand the matching dedup path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/group.rs` around lines 1287 - 1297, The calculation of distinct_mi_count (using templates.iter().filter_map(|t| t.mi.id()).max().map(|m| m+1).unwrap_or(0)) should be extracted into a single helper (e.g., compute_distinct_mi_count or distinct_molecule_id_count) so the contiguity invariant lives in one place; implement the helper to accept an iterator or slice of templates (or a closure to get mi ids) and return u64, replace the inline logic in this file (where distinct_mi_count is computed), in process_and_write_position_group, and in the dedup path (src/lib/commands/dedup.rs) to call the new helper, and add tests or a small doc comment on the helper to codify the max+1 semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/commands/group.rs`:
- Around line 1287-1297: The calculation of distinct_mi_count (using
templates.iter().filter_map(|t| t.mi.id()).max().map(|m| m+1).unwrap_or(0))
should be extracted into a single helper (e.g., compute_distinct_mi_count or
distinct_molecule_id_count) so the contiguity invariant lives in one place;
implement the helper to accept an iterator or slice of templates (or a closure
to get mi ids) and return u64, replace the inline logic in this file (where
distinct_mi_count is computed), in process_and_write_position_group, and in the
dedup path (src/lib/commands/dedup.rs) to call the new helper, and add tests or
a small doc comment on the helper to codify the max+1 semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9403bdf0-210b-4e89-bec5-fd887fe87524
📒 Files selected for processing (4)
src/lib/commands/dedup.rssrc/lib/commands/group.rssrc/lib/grouper.rstests/integration/test_group_command.rs
This branch was previously deployed
1 participant
Summary
fgumi groupandfgumi dedupreserved MI counter blocks sized by template count, but each position group only emits one MI per distinct UMI family (and paired A/B share a numeric id), so the block was almost always larger than needed. Unused slots became gaps in the global MI space (~22% wasted IDs on a 1 M-pair test) and propagated to downstream consensus read names, breaking cross-tool comparison with fgbio.distinct_mi_countonProcessedPositionGroup/ProcessedDedupGroupcomputed asmax(MoleculeId::id()) + 1across assigned templates, and advance the global MI counter by that value instead oftemplates.len(). Fix applies to both the unified parallel pipeline and the single-threaded streaming path.max(MI) == distinct_count - 1, matching fgbio'sGroupReadsByUmi.Fixes #269
Test plan
test_group_command_assigns_consecutive_mi_valuesreproduces the bug: MIs{0, 4, 5, 9}, max 9 vs expected 3.test_group_command_assigns_consecutive_mi_values_multi_threadedcovers the parallel pipeline path with--threads 4and 20 position groups.cargo ci-test: 2495 passed, 19 skipped.cargo ci-fmtclean.cargo ci-lintclean.Scope note
This removes the gaps in the MI integer space (the literal issue). It does not guarantee MI values appear in monotonically increasing order in the output BAM — parallel serialize workers may reserve their (now correctly-sized) blocks in any interleaved order. If byte-for-byte output-order monotonicity to match fgbio is also required, that is a follow-up (option (a) from the issue: end-of-pipeline renumber, or serial MI assignment during output write).