Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #118 +/- ##
==========================================
+ Coverage 88.97% 88.99% +0.02%
==========================================
Files 113 113
Lines 55038 55111 +73
==========================================
+ Hits 48970 49047 +77
+ Misses 6068 6064 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3691c0c to
de39ec6
Compare
b00b34c to
1089920
Compare
de39ec6 to
35d4c03
Compare
1089920 to
9c38dcb
Compare
35d4c03 to
24bb0a2
Compare
9c38dcb to
5efac96
Compare
24bb0a2 to
da20643
Compare
5efac96 to
5c87430
Compare
da20643 to
349829c
Compare
5c87430 to
960f9e0
Compare
nh13
changed the title
960f9e0 to
9a3f6de
Compare
30d5cfb to
b20b3cd
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughA new CLI option 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
@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.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/commands/group.rs (1)
5054-5125: These tests don't actually prove the selector changed paths.Both cases only assert final grouping, so they'd still pass if
parallel_group_min_templateswere ignored and mapped groups always stayed on the sequential edit assigner. A small unit test aroundcreate_umi_assigner()/ theuse_paralleldecision, or a paired-strategy regression case that fails when the parallel branch is taken incorrectly, would lock down the new behavior directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/group.rs` around lines 5054 - 5125, The tests only assert final grouping and don't verify that create_umi_assigner() selects the parallel path when GroupReadsByUmi.parallel_group_min_templates is set; add a focused unit test that constructs the command (or directly calls create_umi_assigner()) with parallel_group_min_templates = Some(1) and with None, then asserts the selector/returned assigner type or a behavior unique to the parallel assigner (e.g., type name, enum variant, or a mocked method call) to prove use_parallel is true for the former and false for the latter; reference GroupReadsByUmi, parallel_group_min_templates, and create_umi_assigner() to locate the code under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/fgumi-dna/src/bitenc.rs`:
- Around line 130-148: The public methods base_at and with_base_at currently use
debug_assert! so bounds and base-range checks are disabled in release builds;
replace the debug_assert! calls in base_at and with_base_at with unconditional
assert! (or otherwise make these checked public wrappers that call private
unchecked helpers) so pos < self.len and base < 4 are always enforced, and keep
the existing bit-masking logic (mask, bit_pos, new_bits) unchanged.
In `@src/commands/group.rs`:
- Around line 422-425: The early return when detecting ParallelPairedAssigner
bypasses paired-UMI preprocessing (the parts.len()!=2 check and the
is_r1_earlier logic) and causes same-string paired UMIs to lose their /A vs /B
distinction and malformed paired UMIs to hit asserts in
ParallelPairedAssigner::assign(); remove this shortcut and ensure the same
preprocessing runs for all assigners: parse the UMI into parts, validate
parts.len() == 2, compute/retain is_r1_earlier, normalize/canonicalize the UMI
as the rest of the code expects, then call assigner.assign() (or
assigner.as_any().downcast_ref::<ParallelPairedAssigner>().unwrap().assign()) so
ParallelPairedAssigner still gets canonicalized input and downstream asserts are
avoided while preserving correct paired-suffix behavior (see
ParallelPairedAssigner and assign()).
- Around line 1555-1576: The create_umi_assigner helper currently constructs
ParallelIdentityAssigner when use_parallel is true, which violates the intended
behavior for Strategy::Identity; change the logic so that if strategy ==
Strategy::Identity you always return the sequential identity assigner (use
strategy.new_assigner_full(effective_edits, 1, index_threshold) or the existing
sequential creation path) regardless of use_parallel, and only construct
ParallelEditAssigner / ParallelAdjacencyAssigner / ParallelPairedAssigner for
non-identity strategies when use_parallel is true; update the conditional in
create_umi_assigner to check Strategy::Identity first and return the sequential
assigner instead of ParallelIdentityAssigner::new.
In `@src/lib/umi/parallel_assigner.rs`:
- Around line 365-369: The parallel assigner currently treats
BitEnc::from_umi_str() returning None as a simple invalid UMI, which causes long
UMIs (>32 bases) to be skipped in parallel clustering; change the logic in the
parallel assigner so that if any UMI in a group fails BitEnc::from_umi_str() (or
its length exceeds BitEnc capacity) the entire group is handled by the
sequential path instead of attempting parallel clustering. Concretely: before
populating umi_counts/umi_to_original (where BitEnc::from_umi_str is called),
scan the group for UMIs that cannot be encoded (or check length >32) and, if any
exist, mark the group to use the sequential fallback (respecting the existing
--parallel-group-min-templates behavior) rather than continuing with parallel
processing; update the code paths around BitEnc::from_umi_str, umi_counts, and
umi_to_original to implement this gate or per-group sequential fallback.
- Around line 1-23: Add the module-level unsafe guard by inserting the crate
attribute to deny unsafe code at the top of the file (before any use statements
or module docs); ensure you add #![deny(unsafe_code)] as the first
non-comment/non-doc line in src/lib/umi/parallel_assigner.rs so the compiler
enforces the repo guideline for unsafe code while keeping the rest of the file
(types like Umi, UmiAssigner and functions using BitEnc, MoleculeId, rayon,
ahash, etc.) unchanged.
---
Nitpick comments:
In `@src/commands/group.rs`:
- Around line 5054-5125: The tests only assert final grouping and don't verify
that create_umi_assigner() selects the parallel path when
GroupReadsByUmi.parallel_group_min_templates is set; add a focused unit test
that constructs the command (or directly calls create_umi_assigner()) with
parallel_group_min_templates = Some(1) and with None, then asserts the
selector/returned assigner type or a behavior unique to the parallel assigner
(e.g., type name, enum variant, or a mocked method call) to prove use_parallel
is true for the former and false for the latter; reference GroupReadsByUmi,
parallel_group_min_templates, and create_umi_assigner() to locate the code under
test.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0340362f-03b5-4fb7-855f-55ca76004138
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/fgumi-dna/Cargo.tomlcrates/fgumi-dna/src/bitenc.rssrc/commands/group.rssrc/lib/umi/mod.rssrc/lib/umi/parallel_assigner.rs
9a3f6de to
1f33928
Compare
…el UMI assignment Add --parallel-group-min-templates N CLI option that enables the parallel UMI assigner (Edit, Adjacency, Paired) for position groups with at least N templates. Decouples parallel assigner from --allow-unmapped so amplicon and other workflows with large mapped position groups can opt in. Extract create_umi_assigner helper to eliminate duplicated assigner selection logic between pipeline and single-threaded code paths.
1f33928 to
11c57e7
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/commands/group.rs (1)
1560-1574:⚠️ Potential issue | 🔴 CriticalKeep
Strategy::Pairedon the sequential assigner for now.Line 411 still returns the raw uppercase UMI for
ParallelPairedAssigner, sois_r1_earliernever reaches paired canonicalization. With this helper, thresholded mapped groups can now lose their/Avs/Bdistinction for same-string paired UMIs.Suggested safe fallback
- if matches!(strategy, Strategy::Identity) { + if matches!(strategy, Strategy::Identity | Strategy::Paired) { return strategy.new_assigner_full(effective_edits, 1, index_threshold); } if use_parallel { match strategy { - Strategy::Identity => unreachable!("handled above"), + Strategy::Identity | Strategy::Paired => unreachable!("handled above"), Strategy::Edit => Box::new(ParallelEditAssigner::new(effective_edits, num_threads)), Strategy::Adjacency => { Box::new(ParallelAdjacencyAssigner::new(effective_edits, num_threads)) } - Strategy::Paired => Box::new(ParallelPairedAssigner::new(effective_edits, num_threads)), } } else { strategy.new_assigner_full(effective_edits, 1, index_threshold) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/group.rs` around lines 1560 - 1574, The ParallelPairedAssigner must not be used; keep paired canonicalization on the sequential assigner. In the match inside the use_parallel branch, replace the Strategy::Paired arm (currently Box::new(ParallelPairedAssigner::new(effective_edits, num_threads))) with a call to the sequential assigner via strategy.new_assigner_full(effective_edits, 1, index_threshold) so Paired falls back to the non-parallel path; leave the other arms unchanged and keep the earlier Identity handling as-is.
🧹 Nitpick comments (1)
src/commands/group.rs (1)
5361-5397: These tests don't prove activation.Both use
test_group_cmd(), so they stay on the fast path; Lines 1283-1292 are still uncovered, and the MI counts here are identical on the sequential and parallel edit paths. A broken branch selection would still pass.Suggested test shape
+ #[test] + fn test_create_umi_assigner_selects_parallel_edit() { + let assigner = create_umi_assigner(Strategy::Edit, 1, 100, 4, true); + assert!(assigner.as_any().is::<ParallelEditAssigner>()); + } + + #[test] + fn test_create_umi_assigner_selects_sequential_edit() { + let assigner = create_umi_assigner(Strategy::Edit, 1, 100, 4, false); + assert!(!assigner.as_any().is::<ParallelEditAssigner>()); + }Also applies to: 5400-5435
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/group.rs` around lines 5361 - 5397, The test is not actually exercising the parallel branch because it still uses test_group_cmd(); fix by constructing and invoking GroupReadsByUmi directly with parallel_group_min_templates set (as done in the test but without using test_group_cmd), or by adding a targeted test that calls the parallel-specific routine (the parallel grouping path) instead of the shared helper; ensure you create input that crosses the parallel threshold and then assert a behavior unique to the parallel path (e.g., run both the sequential and parallel paths and compare outputs or detect the parallel helper invocation) so the code paths for the parallel implementation are truly covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/commands/group.rs`:
- Around line 1560-1574: The ParallelPairedAssigner must not be used; keep
paired canonicalization on the sequential assigner. In the match inside the
use_parallel branch, replace the Strategy::Paired arm (currently
Box::new(ParallelPairedAssigner::new(effective_edits, num_threads))) with a call
to the sequential assigner via strategy.new_assigner_full(effective_edits, 1,
index_threshold) so Paired falls back to the non-parallel path; leave the other
arms unchanged and keep the earlier Identity handling as-is.
---
Nitpick comments:
In `@src/commands/group.rs`:
- Around line 5361-5397: The test is not actually exercising the parallel branch
because it still uses test_group_cmd(); fix by constructing and invoking
GroupReadsByUmi directly with parallel_group_min_templates set (as done in the
test but without using test_group_cmd), or by adding a targeted test that calls
the parallel-specific routine (the parallel grouping path) instead of the shared
helper; ensure you create input that crosses the parallel threshold and then
assert a behavior unique to the parallel path (e.g., run both the sequential and
parallel paths and compare outputs or detect the parallel helper invocation) so
the code paths for the parallel implementation are truly covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9cf6271b-12a6-485b-ae12-67a874ebf9ab
📒 Files selected for processing (1)
src/commands/group.rs
|
Superseded by #371, which adopts the same design (decouple from --allow-unmapped, extract create_umi_assigner, over-subscription warning) but replaces the single user-supplied integer threshold with . The new |
|
(Correction to my previous comment — shell quoting ate the flag name.) Superseded by #371, which keeps the same design from this PR (decouple from The new |
This branch was previously deployed
1 participant
Summary
--parallel-group-min-templates NCLI option that enables the parallel UMI assigner (Edit, Adjacency, Paired) for position groups with at least N templates--allow-unmappedso amplicon and other workflows with large mapped position groups can opt increate_umi_assignerhelper to eliminate duplicated assigner selection logic between pipeline and single-threaded code pathsStacked on #39 (
feat/allow-unmapped-reads).Details
When
--parallel-group-min-templatesis set, groups meeting the threshold use rayon-based parallel edge discovery.--allow-unmappedcontinues to unconditionally use the parallel path (all unmapped reads form one giant group).The CLI docs include a warning about thread over-subscription: the parallel assigner uses rayon's thread pool, which is separate from the pipeline's worker threads (
--threads). For amplicon data (few large groups), pipeline threads are mostly idle so over-subscription is minimal.Test plan
test_parallel_group_min_templates_activates_for_mapped_data— verifies parallel path activates withSome(1)thresholdtest_parallel_group_min_templates_none_uses_sequential— verifies sequential path when threshold isNoneallow_unmappedtests pass unchangedcargo ci-test && cargo ci-fmt && cargo ci-lint(1840 tests passed)