feat(group): add --allow-unmapped flag for processing fully unmapped reads in fgumi group

[nh13]

Summary

• Adds --allow-unmapped flag to fgumi group command to support processing fully unmapped reads (both R1 and R2 unmapped)
• Useful for ribosome display and other protocols that produce unmapped reads with UMI sequences
• When enabled, queryname-sorted input is also accepted (in addition to template-coordinate sorted)
• NEW: Adds parallel UMI assigner for Edit and Adjacency strategies when processing unmapped data

Changes

• Add --allow-unmapped CLI flag to GroupReadsByUmi struct
• Add allow_unmapped field to GroupFilterConfig
• Modify filter_template() to conditionally allow unmapped templates when flag is set
• Relax sort order validation to accept queryname-sorted input when --allow-unmapped is enabled
• Add unit tests for filter_template with unmapped templates
• Add integration tests for grouping unmapped reads with Identity and Adjacency strategies
• Add threading mode tests to verify unmapped reads work correctly in all pipeline configurations
• NEW: Add parallel_assigner module with ParallelEditAssigner and ParallelAdjacencyAssigner
• NEW: Add BitEnc::with_base_at() and base_at() methods for neighbor UMI generation
• NEW: Add runtime warnings when --allow-unmapped is used

Parallel UMI Assigner

When --allow-unmapped is enabled, all unmapped reads form a single position group which can contain millions of unique UMIs. The standard O(U²) algorithms become bottlenecks.

The new parallel assigner uses:

• Parallel edge discovery: Generate all Hamming-1 neighbors and lookup in hash map
• Thread-safe union-find: Concurrent connected component discovery
• Complexity: O(U×L/P) where U=unique UMIs, L=UMI length, P=threads

This provides 10-100x speedup for large unmapped datasets.

Behavior

When --allow-unmapped is enabled:

• Templates with both reads unmapped are no longer filtered out
• Queryname-sorted input is accepted
• Unmapped reads are grouped by UMI only within each library/cell barcode
• All unmapped reads form a single position group (since they have no genomic coordinates)
• Edit and Adjacency strategies use parallel assigner automatically
• Identity strategy unchanged (already O(U) with hash lookups)

Important Notes

⚠️ Over-grouping risk: All unmapped reads are placed in a single position group. Reads with identical/similar UMIs will be grouped together even if they originate from different genomic locations.

⚠️ Paired UMI edit distance: For paired UMIs (e.g., ACGT-TGCA), edit distance is computed on the concatenated sequence with dashes removed. With --edits 1, only 1 mismatch is allowed across ALL bases (30 bases for 15bp-15bp paired UMIs).

Test plan

• Unit tests for filter_template allowing/rejecting unmapped templates
• Integration test for grouping unmapped reads by UMI
• Integration test for adjacency strategy with unmapped reads
• Integration test for mixed mapped/unmapped input
• Integration test verifying unmapped reads are still filtered without the flag
• Parameterized threading tests (single-threaded, 1 thread pipeline, 2 thread pipeline)
• All existing group tests pass with allow_unmapped: false
• Unit tests for parallel assigner (union-find, edge discovery, edit/adjacency equivalence)
• Tests for paired UMIs with dashes in parallel assigner
• Tests for BitEnc::with_base_at() and base_at() methods

[codecov]

Codecov Report

❌ Patch coverage is 95.86276% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.11%. Comparing base (ff845c1) to head (b20b3cd).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/lib/umi/parallel_assigner.rs	96.05%	31 Missing ⚠️
src/commands/group.rs	95.14%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #39      +/-   ##
==========================================
+ Coverage   83.88%   84.11%   +0.23%     
==========================================
  Files         127      128       +1     
  Lines       51100    52082     +982     
==========================================
+ Hits        42866    43810     +944     
- Misses       8234     8272      +38     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[nh13]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds two public BitEnc methods for 2-bit base access and substitution with bounds/value checks. Introduces an allow_unmapped flag to GroupReadsByUmi and threads it through filtering, grouping, MI-tagging, and logging to enable grouping of fully unmapped templates (requires query-name-sorted input when used). Exposes a new umi::parallel_assigner module providing a thread-safe UnionFind, parallel edge discovery, and ParallelIdentity/ParallelEdit/ParallelAdjacency/ParallelPaired assigners. Adds tests for the new behavior and a dev-only test dependency in the DNA crate.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description check	✅ Passed	Description comprehensively covers the changeset: flag purpose, behavior changes, parallel assigner implementation, and important caveats.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title directly and accurately summarizes the primary change: adding an --allow-unmapped flag to fgumi group for processing unmapped reads.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/allow-unmapped-reads

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

crates/fgumi-dna/src/bitenc.rs (1)

261-268: Use rstest for parameterized cases in test_base_at.

Manually asserting each position violates the "use rstest for parameterized tests" guideline. Consider:

♻️ Proposed refactor

+    use rstest::rstest;
+
-    #[test]
-    fn test_base_at() {
-        let enc = BitEnc::from_bytes(b"ACGT").unwrap();
-        assert_eq!(enc.base_at(0), 0); // A
-        assert_eq!(enc.base_at(1), 1); // C
-        assert_eq!(enc.base_at(2), 2); // G
-        assert_eq!(enc.base_at(3), 3); // T
-    }
+    #[rstest]
+    #[case(0, 0)] // A
+    #[case(1, 1)] // C
+    #[case(2, 2)] // G
+    #[case(3, 3)] // T
+    fn test_base_at(#[case] pos: usize, #[case] expected: u8) {
+        let enc = BitEnc::from_bytes(b"ACGT").unwrap();
+        assert_eq!(enc.base_at(pos), expected);
+    }

As per coding guidelines: "Use rstest for parameterized tests."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/fgumi-dna/src/bitenc.rs` around lines 261 - 268, Replace the manual
assertions in the test_base_at function with an rstest parameterized test: keep
constructing the BitEnc via BitEnc::from_bytes(b"ACGT").unwrap(), then
parameterize inputs and expected outputs for base_at using rstest's #[rstest]
attribute (e.g., rows for index and expected base) and assert that
enc.base_at(index) == expected; update the test function signature
(test_base_at) to accept the parameterized index and expected values and remove
the four individual assert_eq! calls.

🤖 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 283-288: The test currently reads bases from enc2 but applies
with_base_at to enc, so it doesn't check idempotence for BitEnc; change the
mutation to use enc2 (i.e., call enc2.with_base_at(pos, base)) and assert the
result equals enc2 (or at least that restored.base_at(pos) == base) — update the
loop that calls BitEnc::from_bytes, base_at, and with_base_at to operate on enc2
instead of enc so the round-trip is properly tested.

In `@src/commands/group.rs`:
- Around line 1197-1217: The parallel branch under allow_unmapped currently
ignores the configured ThreadingOptions num_threads and uses rayon's global
pool; fix by making the parallel path respect num_threads: before creating
ParallelEditAssigner/ParallelAdjacencyAssigner/ParallelPairedAssigner, either
initialize/configure rayon's global pool from ThreadingOptions.num_threads (e.g.
via rayon::ThreadPoolBuilder with num_threads) or short-circuit to the
sequential path (call strategy.new_assigner_full(..., 1, index_threshold)) when
num_threads == 1; update the code where allow_unmapped, num_threads,
Parallel*Assigner::new and strategy.new_assigner_full are used so the chosen
assigner honors the --threads setting (and remove reliance on the deprecated
_threads parameter).

In `@src/lib/umi/parallel_assigner.rs`:
- Around line 469-624: The parallel assigner accepts malformed paired UMIs (no
dash or multiple dashes) whereas the sequential PairedUmiAssigner requires
exactly one dash; add an explicit validation at the start of
ParallelPairedAssigner::assign() that iterates raw_umis and asserts each
umi.split('-').count() == 2 (with a clear panic/error message referencing
ParallelPairedAssigner and paired UMI format) before calling
Self::canonicalize() or BitEnc::from_umi_str; this will make behavior consistent
with the sequential PairedUmiAssigner and avoid silently processing malformed
UMIs.

---

Nitpick comments:
In `@crates/fgumi-dna/src/bitenc.rs`:
- Around line 261-268: Replace the manual assertions in the test_base_at
function with an rstest parameterized test: keep constructing the BitEnc via
BitEnc::from_bytes(b"ACGT").unwrap(), then parameterize inputs and expected
outputs for base_at using rstest's #[rstest] attribute (e.g., rows for index and
expected base) and assert that enc.base_at(index) == expected; update the test
function signature (test_base_at) to accept the parameterized index and expected
values and remove the four individual assert_eq! calls.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR adds parallel UMI assignment capabilities and enhances bit-encoding operations. Changes include: two new BitEnc methods for extracting and replacing bases at specific positions; an experimental allow_unmapped flag in read grouping that accepts additional sort orders and gates parallel UMI assigners; a new parallel_assigner module exposing ParallelEditAssigner, ParallelAdjacencyAssigner, and ParallelPairedAssigner for unmapped reads; and a lock-free UnionFind data structure for parallel graph operations. Tests verify correctness across strategies.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title directly describes the main change: a new --allow-unmapped flag for the fgumi group command to process fully unmapped reads.
Description check	✅ Passed	Description clearly relates to the changeset, detailing the --allow-unmapped flag, parallel UMI assigner, BitEnc methods, and behavioral changes with comprehensive test coverage.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/allow-unmapped-reads

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@crates/fgumi-dna/src/bitenc.rs`:
- Around line 282-287: The test is using the wrong instance (`enc`) when
checking idempotence; change the restoration call to use the instance created
from bytes (`enc2`) so the mutation is exercised. Specifically, in the loop that
calls BitEnc::from_bytes and then uses base_at and with_base_at, replace the
call to enc.with_base_at(pos, base) with enc2.with_base_at(pos, base) (keeping
base_at and the subsequent assert the same) so restored is derived from enc2.

In `@src/commands/group.rs`:
- Around line 1197-1217: The parallel path for allow_unmapped creates
ParallelEditAssigner, ParallelAdjacencyAssigner, and ParallelPairedAssigner with
num_threads, but those constructors currently ignore the threads argument so
--threads has no effect; update each Parallel*Assigner::new implementation to
store and use the threads parameter (e.g., initialize the internal worker pool,
rayon thread pool, or spawn that many workers) or add a method to configure
thread count and call it from the call sites, ensuring
ParallelEditAssigner::new(effective_edits, num_threads),
ParallelAdjacencyAssigner::new(effective_edits, num_threads), and
ParallelPairedAssigner::new(effective_edits, num_threads) actually apply
num_threads; repeat the same fix for the identical code path at the other
location (lines referenced for the duplicate).

In `@src/lib/umi/parallel_assigner.rs`:
- Around line 492-527: The parallel assign path currently accepts malformed
paired UMIs; update ParallelPairedAssigner::assign to validate each raw_umi
before canonicalizing: for each umi in raw_umis, convert to uppercase and ensure
it contains exactly one dash (e.g., use Self::reverse_paired(&upper).is_some()
or split('-') and check parts.len()==2); if the check fails, skip that umi (or
increment an "invalid" counter/log) and do not call Self::canonicalize on it.
This keeps validation consistent with reverse_paired, canonicalize, and
matches_canonical and prevents malformed UMIs from being counted.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/commands/group.rs`:
- Around line 907-934: The code currently accepts queryname-sorted input when
self.allow_unmapped is set, but RecordPositionGrouper assumes
template-coordinate ordering and may mis-group mapped reads; update the check
around is_qname_sorted/is_tc_sorted to either reject queryname-sorted inputs
unless you verify all templates are unmapped or add a runtime guard: when
allow_unmapped && is_qname_sorted, scan a small sample of records (or use an
existing reader/flag) to detect any mapped read (e.g., non-unmapped
FLAG/REFERENCE fields) and if any mapped reads are present, bail with an
explicit error mentioning RecordPositionGrouper and suggesting
template-coordinate sort, otherwise keep the current acceptance path; reference
the symbols is_tc_sorted, is_qname_sorted, self.allow_unmapped and
RecordPositionGrouper to locate where to add the check/warning.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/lib/umi/parallel_assigner.rs`:
- Around line 624-743: ParallelPairedAssigner is missing an explicit override of
split_templates_by_pair_orientation (PairedUmiAssigner provides false) so it
currently uses the trait default (true) and changes grouping behavior; add a
method impl on ParallelPairedAssigner named
split_templates_by_pair_orientation(&self) -> bool that returns false to match
PairedUmiAssigner and preserve existing grouping semantics (this will keep
behavior consistent with umi_for_read_impl and the sequential
PairedUmiAssigner).

---

Duplicate comments:
In `@src/commands/group.rs`:
- Around line 908-935: The code currently accepts queryname-sorted input when
self.allow_unmapped is true (is_qname_sorted), but RecordPositionGrouper assumes
template-coordinate ordering and will break if there are mapped reads; add a
runtime guard that rejects queryname-sorted input unless every template is
unmapped: when is_qname_sorted && self.allow_unmapped, scan the input records
(using the BAM/SAM reader and Record methods such as record.is_unmapped() or
reference_id() on each record) and if you encounter any mapped read
(reference_id != -1 or !is_unmapped()), bail with a clear error (similar style
to existing bail messages) instructing the user to template-coordinate sort or
remove mapped reads; reference the symbols is_qname_sorted, is_tc_sorted,
self.allow_unmapped, and RecordPositionGrouper (and its add_records/finish code
paths) so the check prevents handing mapped records to RecordPositionGrouper.

[coderabbitai]

🤖 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 908-935: When input is queryname-sorted and --allow-unmapped is
set, add a runtime guard to reject the file as soon as any mapped read is
observed because RecordPositionGrouper assumes template-coordinate order; update
the code path that constructs/feeds RecordPositionGrouper (or add the check
inside RecordPositionGrouper) to inspect records and bail with a clear error if
is_qname_sorted && self.allow_unmapped and a record with mapped flags (i.e., not
unmapped for its template) is encountered, referencing
is_template_coordinate_sorted, is_sorted/QUERY_NAME, self.allow_unmapped and
RecordPositionGrouper so callers know to either provide
template-coordinate-sorted BAM or ensure all templates are unmapped.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/commands/group.rs`:
- Around line 891-898: The warning about paired-UMI edit semantics is only shown
for Strategy::Edit and Strategy::Adjacency but should also be shown for
Strategy::Paired; update the conditional guarding the warn(...) block (the
matches! check that currently references Strategy::Edit | Strategy::Adjacency)
to include Strategy::Paired so the message is emitted when self.strategy is
Strategy::Paired.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 35d4c03 and 24bb0a2.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (5)

• crates/fgumi-dna/Cargo.toml
• crates/fgumi-dna/src/bitenc.rs
• src/commands/group.rs
• src/lib/umi/mod.rs
• src/lib/umi/parallel_assigner.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• src/lib/umi/mod.rs