docs(group): clarify the paired strategy never actually uses the index - #856
Conversation
`PairedUmiAssigner::uses_index` is documented as "the paired strategy indexes at every `--edits` value", and the size gate it reads does return true above the threshold. But a paired canonical form always carries the `-` separator between its halves, and `-` is a non-`ACGT` byte, so `NgramIndex::new` and `BkTree::from_umis` (which encode only `ACGT`) invariably decline it. The paired strategy therefore always falls back to its reverse-aware linear scan, at every pool size -- the index is never actually built for paired UMIs. The docs implied the opposite, which is misleading about both performance (large paired pools get no index acceleration) and correctness. Correct the `uses_index` doc (and the sibling test doc) to say the paired strategy is index- *eligible* by size but never realizes it, and add `test_paired_forms_are_never_indexed_because_of_the_dash` pinning that `NgramIndex::new` / `BkTree::from_umis` decline dash-delimited forms. That test is a deliberate guard: the indexed candidate search (`find_within`) is forward-Hamming only, so a future change that taught the index to accept dash-delimited forms (e.g. stripping the dash for a perf win) would silently drop the reverse-orientation (`A-B` == `B-A`) edges the paired matcher draws -- such a change must make the indexed path reverse-aware, and this test forces that reckoning.
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (1)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughThe change clarifies that paired UMI index eligibility does not guarantee index use. Dash-delimited paired UMIs are rejected by ChangesPaired UMI index behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR clarifies paired-strategy documentation and adds a regression test without changing runtime behavior; no actionable merge-blocking risk remains beyond normal checks. Suggested labels: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #856 +/- ##
==========================================
- Coverage 94.51% 94.50% -0.01%
==========================================
Files 193 193
Lines 120001 120008 +7
==========================================
+ Hits 113416 113419 +3
- Misses 6585 6589 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was successfully deployed
1 participant
Summary
PairedUmiAssigner::uses_indexis documented as "the paired strategy indexes at every--editsvalue — N-gram for k=1, BK-tree for k>1", and the size gate it reads does returntrueabove the threshold. But a paired canonical form always carries the-separator between its halves, and-is a non-ACGTbyte, soNgramIndex::newandBkTree::from_umis— which encode onlyACGT— invariably decline it. The paired strategy therefore always falls back to its reverse-aware linear scan, at every pool size: the index is never actually built for paired UMIs. The docs implied the opposite, which is misleading about both performance (large paired pools get no index acceleration) and correctness.What changed
Corrects the
uses_indexdoc and the sibling test doc to say the paired strategy is index-eligible by size but never realizes it, and addstest_paired_forms_are_never_indexed_because_of_the_dashpinning thatNgramIndex::new/BkTree::from_umisdecline dash-delimited forms.That test is a deliberate guard, not just a characterization: the indexed candidate search (
find_within) is forward-Hamming only, so a future change that taught the index to accept dash-delimited forms (e.g. stripping the dash for a perf win on large paired pools) would silently drop the reverse-orientation (A-B==B-A) edges the paired matcher draws. Such a change must make the indexed path reverse-aware first, and this test forces that reckoning.Context
Surfaced while investigating (and dismissing) a CodeRabbit finding on #852 that claimed the parallel paired assigner diverges from the sequential one at ≥100 unique forms — it doesn't, precisely because the sequential paired path never indexes. This documents that invariant so the next person doesn't have to rediscover it. Docs + test only; no behavior change.
Risk: command output changes—none;
unsafechanges—none, and theCLAUDE.mdallowlist is unchanged; memory bounds, queue capacity, and thread/backpressure policy changes—none.Fix: Clarify paired UMI index eligibility and document fallback to reverse-aware linear matching.
NgramIndexandBkTree.