Repository navigation
feat(group): accept --index-threshold always|never, reject unsatisfiable requests - #632
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 (7)
WalkthroughThe PR replaces numeric index thresholds with ChangesIndex threshold configuration and enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Command
participant Strategy
participant Validator
participant Assigner
Command->>Strategy: resolve strategy and edits
Command->>Validator: validate IndexThreshold
Validator->>Strategy: check can_use_index(edits)
Validator-->>Command: accept or reject configuration
Command->>Assigner: construct with IndexThreshold
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 #632 +/- ##
========================================
Coverage 93.65% 93.66%
========================================
Files 175 176 +1
Lines 107601 107772 +171
========================================
+ Hits 100774 100943 +169
- Misses 6827 6829 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4d70bdd to
b27997d
Compare
b27997d to
dc5e8ba
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.
|
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 `@src/lib/commands/dedup.rs`:
- Around line 1123-1129: The --index-threshold help text incorrectly equates 0
with always beyond gating. Update the documentation on the index_threshold
option in src/lib/commands/dedup.rs lines 1123-1129 and
src/lib/commands/group.rs lines 861-867 to clarify that 0 and always agree only
for gating, while only always asserts and errors when the strategy cannot index;
leave the option behavior unchanged.
🪄 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: 50a4b0da-08e7-4c20-9b33-7f5f221d7815
📒 Files selected for processing (7)
crates/fgumi-umi/src/assigner.rscrates/fgumi-umi/src/index_threshold.rscrates/fgumi-umi/src/lib.rssrc/lib/commands/common.rssrc/lib/commands/dedup.rssrc/lib/commands/group.rstests/integration/test_dedup_command.rs
The `--index-threshold` help was silent about a second gate. `build_adjacency_graph_bitenc` requires `self.max_mismatches == 1`, so the Adjacency strategy ignores `--index-threshold` entirely at any other `--edits` and falls back to the O(u^2) scan. The Paired strategy has no such restriction: its generic `build_adjacency_graph` indexes at every edit distance, N-gram for k=1 and BK-tree for k>1. Rather than only rewording the help, extract the gate into `AdjacencyUmiAssigner::uses_index` and `PairedUmiAssigner::uses_index` and call the former from `build_adjacency_graph_bitenc`. The condition now has a name, a doc comment, and three rstest tables covering the threshold boundary, `--edits` sensitivity, and the paired strategy's wider coverage -- so the help is checked against behaviour instead of restating it. The help also still read as though the threshold were a switch. It is a minimum: the gate is `distinct >= threshold`, so `0` indexes every position group rather than disabling the index, and no small value turns the index off -- that takes a threshold larger than any group. Anyone reaching for `0` to force a linear scan, to isolate the index in a profile or to sidestep a suspected index bug, gets maximum indexing instead. The help now says so outright. Behaviour is unchanged; this is documentation plus the extraction needed to test it.
1bbb9fa to
d6d655e
Compare
d6d655e to
b31d8e2
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
b31d8e2 to
6521aa1
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/commands/group.rs (1)
482-501: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winParallel assigners ignore
index_thresholdentirely.
ParallelEditAssigner/ParallelAdjacencyAssigner/ParallelPairedAssignertake no threshold, so under--threads Nwith a parallel-eligible group--index-threshold alwayspasses validation and is then discarded.validate_index_thresholdcan't see the per-groupuse_paralleldecision, so at minimum say so in the--index-thresholdhelp (Lines 861-868) — currently it reads as unconditional.🤖 Prompt for 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. In `@src/lib/commands/group.rs` around lines 482 - 501, The parallel branch of create_umi_assigner drops index_threshold when constructing ParallelEditAssigner, ParallelAdjacencyAssigner, and ParallelPairedAssigner. Update the --index-threshold help text near its definition to state that the setting is ignored for groups using parallel assigners, while preserving existing validation and assignment behavior.src/lib/commands/common.rs (1)
135-175: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEdit's log line will lie at
--edits != 1if thecan_indexgate stays.The Adjacency arm correctly reports "not used" outside
--edits 1; Edit unconditionally prints a floored number even thoughSimpleErrorUmiAssigner::assignnever reaches the index at--edits != 1(crates/fgumi-umi/src/assigner.rsLine 1290). Downstream of that root cause — no change needed here if the gate is dropped.🤖 Prompt for 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. In `@src/lib/commands/common.rs` around lines 135 - 175, Remove the `can_index` gate that prevents the Edit strategy from reaching the index when effective edits differ from 1, so `SimpleErrorUmiAssigner::assign` uses the index consistently with `index_threshold_log_message`. Preserve the existing floored threshold reporting and Adjacency-specific “not used” behavior.crates/fgumi-umi/src/assigner.rs (1)
1285-1296: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOne root cause:
SimpleErrorUmiAssigner::assigngates the whole defer/index branch oncan_index = max_mismatches == 1, while everything else this PR adds says Edit indexes at every--editsvalue. Consequence:--index-threshold always --strategy edit --edits 2passesvalidate_index_thresholdand then silently scans — the quietly-ignored-flag failureIndexThresholdwas introduced to eliminate. Pick one contract (drop the gate, or narrow Edit toedits == 1) and make these five sites agree.
crates/fgumi-umi/src/assigner.rs#L1285-L1296: dropcan_index = self.max_mismatches == 1(let it betrue) so the index is reachable at every distance —NgramIndex::newalready declines inputs it cannot partition — or keep it and narrow everything below.crates/fgumi-umi/src/assigner.rs#L600-L617: if the gate stays, changeStrategy::Edit | Strategy::Paired => trueto give Edit its ownedits == 1arm.crates/fgumi-umi/src/assigner.rs#L4852-L4884: drivetest_edit_index_is_built_at_every_edit_distancethroughassign()instead of callingcomponents_via_indexdirectly, so the claim is actually exercised; likewise maketest_edit_index_matches_scan's edits=2/3 cases non-vacuous.src/lib/commands/common.rs#L135-L175: if the gate stays, give theStrategy::Editarm the same "not used" branch the Adjacency arm has foreffective_edits != 1, and update the pinnededit_two_edits/edit_zero_mismatchescases at Lines 1300-1311.src/lib/commands/dedup.rs#L1123-L1132: qualify "Edit, Adjacency and Paired index" if Edit ends up gated to--edits 1.src/lib/commands/group.rs#L861-L868: apply the identical help-text change (same string).🤖 Prompt for 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. In `@crates/fgumi-umi/src/assigner.rs` around lines 1285 - 1296, Make Edit indexing reachable for every edit distance by removing the max_mismatches == 1 gate in SimpleErrorUmiAssigner::assign at crates/fgumi-umi/src/assigner.rs:1285-1296; retain NgramIndex::new as the capability check. Update the Strategy handling at crates/fgumi-umi/src/assigner.rs:600-617 to preserve Edit indexing for all edit values, and revise the tests at crates/fgumi-umi/src/assigner.rs:4852-4884 to exercise assign() and make edits=2/3 cases non-vacuous. No direct changes are required at src/lib/commands/common.rs:135-175, src/lib/commands/dedup.rs:1123-1132, or src/lib/commands/group.rs:861-868 because the all-edit-distance Edit contract remains unchanged.
🤖 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 `@tests/integration/test_dedup_command.rs`:
- Around line 531-533: The dedup integration test only verifies that an output
file exists, not that the never-threshold output preserves the default-threshold
result. Update the test around cmd.execute("fgumi dedup") to read the output BAM
and assert all 6 records with their expected MI grouping, or compare it against
a default-threshold run using the same input.
---
Outside diff comments:
In `@crates/fgumi-umi/src/assigner.rs`:
- Around line 1285-1296: Make Edit indexing reachable for every edit distance by
removing the max_mismatches == 1 gate in SimpleErrorUmiAssigner::assign at
crates/fgumi-umi/src/assigner.rs:1285-1296; retain NgramIndex::new as the
capability check. Update the Strategy handling at
crates/fgumi-umi/src/assigner.rs:600-617 to preserve Edit indexing for all edit
values, and revise the tests at crates/fgumi-umi/src/assigner.rs:4852-4884 to
exercise assign() and make edits=2/3 cases non-vacuous. No direct changes are
required at src/lib/commands/common.rs:135-175,
src/lib/commands/dedup.rs:1123-1132, or src/lib/commands/group.rs:861-868
because the all-edit-distance Edit contract remains unchanged.
In `@src/lib/commands/common.rs`:
- Around line 135-175: Remove the `can_index` gate that prevents the Edit
strategy from reaching the index when effective edits differ from 1, so
`SimpleErrorUmiAssigner::assign` uses the index consistently with
`index_threshold_log_message`. Preserve the existing floored threshold reporting
and Adjacency-specific “not used” behavior.
In `@src/lib/commands/group.rs`:
- Around line 482-501: The parallel branch of create_umi_assigner drops
index_threshold when constructing ParallelEditAssigner,
ParallelAdjacencyAssigner, and ParallelPairedAssigner. Update the
--index-threshold help text near its definition to state that the setting is
ignored for groups using parallel assigners, while preserving existing
validation and assignment behavior.
🪄 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: 77fc4353-2d1b-4e94-9442-8292f699205f
📒 Files selected for processing (7)
crates/fgumi-umi/src/assigner.rscrates/fgumi-umi/src/index_threshold.rscrates/fgumi-umi/src/lib.rssrc/lib/commands/common.rssrc/lib/commands/dedup.rssrc/lib/commands/group.rstests/integration/test_dedup_command.rs
…ble requests (#636) `--index-threshold` was a bare `usize` meaning "index once a position group holds at least this many distinct UMIs". That left no way to turn the index off: suppressing it needed a value larger than any group, i.e. literally `--index-threshold 18446744073709551615`. The previous commit spelled out that `0` enables the index for every group rather than disabling it — but saying so plainly only made the missing capability more obvious. Make the option a keyword-or-number, matching `--parallel-group-min-templates` one screen away in the same file: --index-threshold always index every position group --index-threshold never always scan all UMI pairs --index-threshold <N> index groups of N or more (default 100) `never` is the new capability. `always` is sugar for `0`, which already meant this but read as its opposite — the trap that produced the wrong docs in the first place. Every existing integer invocation keeps working unchanged, `0` included, so no command line silently changes meaning. Keeping this in one option rather than adding a `--no-index` flag makes the contradictory state unrepresentable: two knobs controlling one behaviour would need a precedence rule for `--no-index --index-threshold 50`. `always` asserts that indexing will happen, so a configuration that can never index is now a command-line error rather than a flag that is quietly ignored: $ fgumi group --strategy identity --index-threshold always Error: --index-threshold always cannot be honoured with --strategy identity: the identity strategy never uses the UMI index. ... $ fgumi group --strategy adjacency --edits 2 --index-threshold always Error: --index-threshold always cannot be honoured with --strategy adjacency --edits 2: the adjacency strategy only indexes at --edits 1. ... That second case surfaces an asymmetry the help had left implicit: the edit and adjacency strategies silently ignore the index at any edit distance other than 1, while paired indexes at all of them. `Strategy::can_use_index` now states it, and the check runs against the EFFECTIVE strategy and edits so `--no-umi` (which forces identity) is caught too. A bare integer is deliberately not checked. It is a tuning knob allowed to end up inert — otherwise the default `100` could not coexist with `--strategy identity` — and that includes `0`, even though `0` admits every group exactly as `always` does. `Edit` grew an index of its own in #645, gated on its own measured crossover (`EDIT_INDEX_THRESHOLD`, 200) rather than the shared default. `IndexThreshold::floored_at` expresses that floor: a numeric threshold is raised to the crossover, while the keywords pass through untouched, so `always` still means every group — the escape hatch that isolates the index in a profile — and `never` still means none. Every existing numeric invocation keeps the `max(flag, 200)` behaviour it had. What `Strategy::can_use_index` reports for `Edit` is `edits == 1`, the same as adjacency. It is tempting to say otherwise: `components_via_index` hands `max_mismatches` straight to `NgramIndex::new`, which partitions each UMI into `max_mismatches + 1` pieces and pigeonholes over them, so the index is *capable* at any distance — a new test pins that, brute-force, across five UMI lengths and four distances. But `assign` never *reaches* it elsewhere: #645 gated the defer/index branch on one mismatch deliberately, because `EDIT_INDEX_THRESHOLD`'s crossover was measured there and nowhere else. Past it the index loses — the partitions get too short to be selective (4 bases at k=1, 2 at k=2 for an 8-base UMI) while single linkage collapses the group into one component, making the set-merge's early-exiting scan cheaper rather than dearer. Widening the gate is a benchmarking question; `benches/umi_assigner_threshold.rs` sweeps one mismatch only. So both `edits == 1` gates now read from the assigner that enforces them — `SimpleErrorUmiAssigner::indexes_at_edit_distance` and its adjacency twin — rather than being restated in `can_use_index`, in the `Index threshold:` startup line, and in the parity test's rationale. Restating them is how they came to disagree in the first place, and the parity test is back to sweeping one mismatch: at any other distance both sides run the identical set-merge, so sweeping wider compared the scan against itself. `dedup` carries its own copy of the option, so the validation lives in `commands::common` and both call it. Assigner constructors take `T: Into<IndexThreshold>`, leaving every existing integer call site untouched.
cd4df0c to
e6405f6
Compare
|
All four findings from the last review are addressed in
You were right that five sites disagreed with one gate. Picking which side to keep took some digging, and the gate wins:
You also noted this is not a correctness question, and that is right: So both Beyond the five sites you listed, four more needed the same correction. One is a bug in its own right:
The others: the
|
|
@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.
|
What this does
--index-thresholdwas a bareusizemeaning "index once a position group holds at least this many distinct UMIs". That left no way to turn the index off: suppressing it needed a value larger than any group, i.e. literally--index-threshold 18446744073709551615.This makes the option a keyword-or-number, matching
--parallel-group-min-templatesone screen away in the same file:neveris the new capability.alwaysis sugar for0, which already meant this but read as its opposite — the trap that produced the wrong help text in the first place. Every existing integer invocation keeps working unchanged,0included, so no command line silently changes meaning.Keeping this in one option rather than adding a
--no-indexflag makes the contradictory state unrepresentable: two knobs controlling one behaviour would need a precedence rule for--no-index --index-threshold 50.alwaysis an assertion, so it can failA configuration that can never index is now a command-line error rather than a flag that is quietly ignored:
That second case surfaces an asymmetry the help had left implicit:
build_adjacency_graph_bitencgates onmax_mismatches == 1andSimpleErrorUmiAssigner::assigngates its defer/index branch the same way, so the adjacency and edit strategies both silently ignore the index at any other--edits, while paired indexes at all of them.Strategy::can_use_indexnow states it, and the check runs against the effective strategy and edits so--no-umi(which forces identity) is caught too.A bare integer is deliberately not checked. It is a tuning knob allowed to end up inert — otherwise the default
100could not coexist with--strategy identity— and that includes0, even though0admits every group exactly asalwaysdoes.dedupcarries its own copy of the option, so the validation lives incommands::commonand both call it. Assigner constructors takeT: Into<IndexThreshold>, leaving every existing integer call site untouched.Integrating with edit's own index (#645)
Editgrew an index of its own in #645, gated on its own measured crossover (EDIT_INDEX_THRESHOLD, 200) rather than the shared default.IndexThreshold::floored_atexpresses that floor: a numeric threshold is raised to the crossover, while the keywords pass through untouched, soalwaysstill means every group — the escape hatch that isolates the index in a profile — andneverstill means none. Every existing numeric invocation keeps themax(flag, 200)behaviour it had.What
Strategy::can_use_indexreports forEditisedits == 1, the same as adjacency. It is tempting to say otherwise:components_via_indexhandsmax_mismatchesstraight toNgramIndex::new, which partitions each UMI intomax_mismatches + 1pieces and pigeonholes over them, so the index is capable at any distance — a new brute-force test pins that across five UMI lengths and four distances, including the lengthsmax_mismatches + 1does not divide.But
assignnever reaches it elsewhere. #645 gated the defer/index branch on one mismatch deliberately, becauseEDIT_INDEX_THRESHOLD's crossover was measured there and nowhere else, and past it the index loses: the partitions get too short to be selective (4 bases at k=1, 2 at k=2 for an 8-base UMI) while single linkage collapses the group into one component, which makes the set-merge's early-exiting scan cheaper rather than dearer. Widening the gate is a benchmarking question —benches/umi_assigner_threshold.rssweeps one mismatch only.So both
edits == 1gates now read from the assigner that enforces them,SimpleErrorUmiAssigner::indexes_at_edit_distanceand its adjacency twin, rather than being restated incan_use_index, in theIndex threshold:startup line, and in the parity test's rationale. Restating them is how they came to disagree in the first place: the index's capability at k>1 was read as evidence thatassignused it there. The parity test stays at one mismatch — at any other distance both sides run the identical set-merge, so sweeping wider would compare the scan against itself.The gate has a name now
Rather than only rewording the help, the first commit extracts the condition so it can be tested rather than restated:
AdjacencyUmiAssigner::uses_index(num_umis)— the threshold and themax_mismatches == 1restriction;build_adjacency_graph_bitenccalls it.PairedUmiAssigner::uses_index(num_umis)— the threshold only, reading the sameindex_admitsgatebuild_adjacency_graphruns rather than a parallel copy of it.SimpleErrorUmiAssigner::uses_index(num_umis)— the threshold only, added here alongside edit's integration.Tests
index_threshold.rs— parsing (both keywords, mixed case, integers, rejected input naming the accepted forms),admits,demands_indexing,floored_at, andDisplayround-trips.test_strategy_can_use_index— the strategy/edits matrix that decides whetheralwaysis honourable.test_edit_index_is_built_at_every_edit_distance— pinned against a realNgramIndexbuild at 0–3 mismatches, not against the flag.test_edit_index_matches_scan—alwaysvsneverproduce identical molecule ids, swept across 1–3 mismatches. Naming the two sides as keywords also removes the old failure mode where a pair of numbers could silently compare the scan against itself.test_index_threshold_always_rejected_when_index_unreachable/..._accepted_when_satisfiableongroup, and the equivalent end-to-end pair ondedup.Full suite green: 6709 passed, plus
ci-fmt,ci-lint, and docs withRUSTDOCFLAGS=-D warnings.Not addressed here
--edits > 1has no index on the adjacency path. The BK-tree branch that would serve k>1 exists but is only reachable from the paired strategy's generic builder, so--edits 2inherits the O(u²) behaviour on large position groups.--index-threshold alwaysnow reports this as an error rather than ignoring it, which is the point — but wiring the BK-tree into the BitEnc path is a separate change and wants a benchmark first.Summary by CodeRabbit
IndexThresholdconfiguration for--index-threshold(always,never, or minimum UMI-group size) with consistent indexing eligibility across strategies and edit distances.--index-threshold alwayscombinations are now rejected with clear, strategy-specific error messages instead of being ignored.--edits 1where applicable).--index-thresholdsemantics, including adjacency constraints andNeverhandling.