Repository navigation
feat(group): accept --index-threshold always|never, reject unsatisfiable requests - #636
Merged
Merged
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)
WalkthroughAdds ChangesUMI index-threshold control
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DedupOrGroupCommand
participant validate_index_threshold
participant Strategy
User->>DedupOrGroupCommand: provide --index-threshold
DedupOrGroupCommand->>validate_index_threshold: validate threshold, strategy, edits
validate_index_threshold->>Strategy: can_use_index(edits)
Strategy-->>validate_index_threshold: index eligibility
validate_index_threshold-->>DedupOrGroupCommand: success or configuration error
Suggested labels: 🚥 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 @@
## nh/fix-index-threshold-doc #636 +/- ##
==============================================================
- Coverage 93.57% 93.55% -0.03%
==============================================================
Files 175 176 +1
Lines 106770 106851 +81
==============================================================
+ Hits 99912 99963 +51
- Misses 6858 6888 +30 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
nh13
force-pushed
the
nh/fix-index-threshold-doc
branch
from
July 23, 2026 23:05
4d70bdd to
b27997d
Compare
nh13
force-pushed
the
nh/feat-index-threshold-never
branch
from
July 23, 2026 23:05
f40466c to
c69569b
Compare
nh13
force-pushed
the
nh/fix-index-threshold-doc
branch
from
July 24, 2026 00:39
b27997d to
dc5e8ba
Compare
…ble requests
`--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
corrected the help text, which had claimed `0` disabled the index when it
in fact enables it for every group — but correcting the docs removed a
documented capability rather than delivering it.
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. 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
adjacency strategy silently ignores the index at any edit distance other
than 1, while the paired strategy 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.
`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 all 41 existing integer call sites
untouched.
nh13
force-pushed
the
nh/feat-index-threshold-never
branch
from
July 24, 2026 00:40
c69569b to
b916451
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Jul 25, 2026
nh13
added a commit
that referenced
this pull request
Jul 25, 2026
…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 adjacency strategy silently ignores the index at any edit distance other than 1, while the edit and paired strategies index 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. That also settles what `Strategy::can_use_index` reports for `Edit`: it indexes, and at every edit distance. `components_via_index` hands `max_mismatches` straight to `NgramIndex::new`, which partitions each UMI into `max_mismatches + 1` pieces and pigeonholes over them, so unlike adjacency there is no one-mismatch gate. Three places said otherwise — the module doc, the `Index threshold:` startup line, and the index/scan parity test's stated reason for sweeping only one mismatch. All three are corrected, and that parity test now sweeps one through three mismatches, where it previously would have compared the scan against itself had the claim been true. `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.
nh13
added a commit
that referenced
this pull request
Jul 25, 2026
…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 adjacency strategy silently ignores the index at any edit distance other than 1, while the edit and paired strategies index 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. That also settles what `Strategy::can_use_index` reports for `Edit`: it indexes, and at every edit distance. `components_via_index` hands `max_mismatches` straight to `NgramIndex::new`, which partitions each UMI into `max_mismatches + 1` pieces and pigeonholes over them, so unlike adjacency there is no one-mismatch gate. Three places said otherwise — the module doc, the `Index threshold:` startup line, and the index/scan parity test's stated reason for sweeping only one mismatch. All three are corrected, and that parity test now sweeps one through three mismatches, where it previously would have compared the scan against itself had the claim been true. `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.
nh13
added a commit
that referenced
this pull request
Jul 25, 2026
…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 adjacency strategy silently ignores the index at any edit distance other than 1, while the edit and paired strategies index 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. That also settles what `Strategy::can_use_index` reports for `Edit`: it indexes, and at every edit distance. `components_via_index` hands `max_mismatches` straight to `NgramIndex::new`, which partitions each UMI into `max_mismatches + 1` pieces and pigeonholes over them, so unlike adjacency there is no one-mismatch gate. Three places said otherwise — the module doc, the `Index threshold:` startup line, and the index/scan parity test's stated reason for sweeping only one mismatch. All three are corrected, and that parity test now sweeps one through three mismatches, where it previously would have compared the scan against itself had the claim been true. `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.
nh13
added a commit
that referenced
this pull request
Jul 25, 2026
…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 adjacency strategy silently ignores the index at any edit distance other than 1, while the edit and paired strategies index 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. That also settles what `Strategy::can_use_index` reports for `Edit`: it indexes, and at every edit distance. `components_via_index` hands `max_mismatches` straight to `NgramIndex::new`, which partitions each UMI into `max_mismatches + 1` pieces and pigeonholes over them, so unlike adjacency there is no one-mismatch gate. Three places said otherwise — the module doc, the `Index threshold:` startup line, and the index/scan parity test's stated reason for sweeping only one mismatch. All three are corrected, and that parity test now sweeps one through three mismatches, where it previously would have compared the scan against itself had the claim been true. `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.
nh13
added a commit
that referenced
this pull request
Jul 25, 2026
…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 adjacency strategy silently ignores the index at any edit distance other than 1, while the edit and paired strategies index 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. That also settles what `Strategy::can_use_index` reports for `Edit`: it indexes, and at every edit distance. `components_via_index` hands `max_mismatches` straight to `NgramIndex::new`, which partitions each UMI into `max_mismatches + 1` pieces and pigeonholes over them, so unlike adjacency there is no one-mismatch gate. Three places said otherwise — the module doc, the `Index threshold:` startup line, and the index/scan parity test's stated reason for sweeping only one mismatch. All three are corrected, and that parity test now sweeps one through three mismatches, where it previously would have compared the scan against itself had the claim been true. `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.
nh13
added a commit
that referenced
this pull request
Jul 25, 2026
…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 adjacency strategy silently ignores the index at any edit distance other than 1, while the edit and paired strategies index 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. That also settles what `Strategy::can_use_index` reports for `Edit`: it indexes, and at every edit distance. `components_via_index` hands `max_mismatches` straight to `NgramIndex::new`, which partitions each UMI into `max_mismatches + 1` pieces and pigeonholes over them, so unlike adjacency there is no one-mismatch gate. Three places said otherwise — the module doc, the `Index threshold:` startup line, and the index/scan parity test's stated reason for sweeping only one mismatch. All three are corrected, and that parity test now sweeps one through three mismatches, where it previously would have compared the scan against itself had the claim been true. `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.
nh13
added a commit
that referenced
this pull request
Jul 25, 2026
…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 adjacency strategy silently ignores the index at any edit distance other than 1, while the edit and paired strategies index 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. That also settles what `Strategy::can_use_index` reports for `Edit`: it indexes, and at every edit distance. `components_via_index` hands `max_mismatches` straight to `NgramIndex::new`, which partitions each UMI into `max_mismatches + 1` pieces and pigeonholes over them, so unlike adjacency there is no one-mismatch gate. Three places said otherwise — the module doc, the `Index threshold:` startup line, and the index/scan parity test's stated reason for sweeping only one mismatch. All three are corrected, and that parity test now sweeps one through three mismatches, where it previously would have compared the scan against itself had the claim been true. `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.
nh13
added a commit
that referenced
this pull request
Jul 26, 2026
…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.
nh13
added a commit
that referenced
this pull request
Jul 26, 2026
…ble requests (#632) * docs(group): pin --index-threshold's semantics behind a named gate 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. * feat(group): accept --index-threshold always|never, reject unsatisfiable 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.
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #632 — review that first; this PR's diff is against it.
Why
#632 corrected the
--index-thresholdhelp, which had claimed "Set to 0 to always use linear scan" when0in fact indexes every group. But correcting the docs removed a documented capability rather than delivering it: the gate isnum_umis >= index_threshold, so there is no value that disables the index short of one larger than any position group — literally--index-threshold 18446744073709551615.What
Make the option a keyword-or-number, matching
--parallel-group-min-templatesone screen away in the same file (Auto | Fixed(usize), with a hand-writtenFromStr):neveris the new capability.alwaysis sugar for0, which already meant this but read as its opposite — precisely the trap that produced the wrong docs. Oncealwaysexists, nobody needs to write or interpret a bare0again.Backward compatible: every existing integer invocation keeps working unchanged,
0included. No one's command line silently flips meaning — which is the decisive argument against the alternative of redefining0to mean "off".Why not
--no-index? Two knobs controlling one behaviour needs a precedence rule for--no-index --index-threshold 50. Keeping it in one option makes the contradictory state unrepresentable. (--index-threshold -1was also considered: it needs anisizewidening plusallow_negative_numbers, and puts a third magic value into an integer whose existing magic value already caused this bug.)Unsatisfiable requests are now errors
alwaysasserts that indexing will happen, so a configuration that can never index is rejected rather than accepted and ignored:That second case surfaces an asymmetry the help had left implicit and #632 could only describe: adjacency silently ignores the index at any
--editsother than 1, while paired indexes at all of them.Strategy::can_use_index(edits)now states it in code.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. That includes0, even though0admits every group exactly asalwaysdoes. This asymmetry is the one judgement call in the PR; the alternative (erroring on0too) is defensible and I'll change it if you prefer.Structure
IndexThresholdlives infgumi-uminext to the assigners that consult it, withFromStr,Display(round-trips),Default,From<usize>,admits()anddemands_indexing().dedupcarries its own copy of the option, sovalidate_index_thresholdlives incommands::commonand both commands call it.T: Into<IndexThreshold>, so all 41 existing integer call sites are untouched.Tests
admits()across the boundary,demands_indexing(), andDisplayround-trip.#[case::usize_max_never_indexes(usize::MAX, …)]becomes#[case::never_skips_huge_group(IndexThreshold::Never, …)], which is what that case was always trying to say — plus a new case provingneverwins at every edit distance for both strategies.grouprejectsalwaysunder identity/edit/adjacency-with-edits≠1 and accepts every satisfiable combination;dedupgets the same two tests throughtry_parse_from, since it has its own copy of the flag.Full suite green: 5746 passed, plus
ci-fmt,ci-lint,ci-doc(RUSTDOCFLAGS=-D warnings) andci-doctest.Summary by CodeRabbit
New Features
--index-thresholdsettings: minimum UMI count,always, ornever.dedupandgroupcommands.Bug Fixes
Tests