feat(spectrum): RC-downward truncated reseed interval; swap into FFI - #30
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 36 minutes and 45 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces ChangesRC Truncation Algorithm Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
prmi/src/index/spectrum.rs (1)
2150-2236: ⚡ Quick winExtract the RC downward-walk core shared with
mem_search_backward_truncated_span_rc.This block duplicates the same
qconstruction, in-interval seeding, cappedexpand, and downward walk already implemented inmem_search_backward_truncated_span_rc. Keeping two copies of the byte-identity-critical walk makes future fixes easy to land in one path but not the other. A small helper that returnsL*would keep the span and interval APIs in lockstep.🤖 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 `@prmi/src/index/spectrum.rs` around lines 2150 - 2236, This block duplicates the RC downward-walk logic from mem_search_backward_truncated_span_rc (the q construction, q_key/tokenize_32mer, forward_maximal_len call, seed selection via shares_prefix, the expand logic and the downward l-loop that calls mem_search/recover), so extract that core into a single helper (e.g., rc_downward_walk or compute_rc_star) that accepts the prepared q slice (or q_start/anchor_end/read), anchor_len/anchor_end, sa_num, pac, enc, l_pac and min_intv and returns the chosen MemMatch (or the L* span so callers can recover as needed); replace the duplicated code here (the q construction + q_key, seed derivation, expand closure, downward walk and recover call) with a call to the new helper and reuse the same helper from mem_search_backward_truncated_span_rc to keep both paths in sync.
🤖 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.
Nitpick comments:
In `@prmi/src/index/spectrum.rs`:
- Around line 2150-2236: This block duplicates the RC downward-walk logic from
mem_search_backward_truncated_span_rc (the q construction, q_key/tokenize_32mer,
forward_maximal_len call, seed selection via shares_prefix, the expand logic and
the downward l-loop that calls mem_search/recover), so extract that core into a
single helper (e.g., rc_downward_walk or compute_rc_star) that accepts the
prepared q slice (or q_start/anchor_end/read), anchor_len/anchor_end, sa_num,
pac, enc, l_pac and min_intv and returns the chosen MemMatch (or the L* span so
callers can recover as needed); replace the duplicated code here (the q
construction + q_key, seed derivation, expand closure, downward walk and recover
call) with a call to the new helper and reuse the same helper from
mem_search_backward_truncated_span_rc to keep both paths in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a239ad6b-709f-4ab6-a6a3-966e5eb3da2b
📒 Files selected for processing (2)
prmi-sys/src/lib.rsprmi/src/index/spectrum.rs
Add `mem_search_backward_truncated_interval_rc`, the fast twin of `mem_search_backward_truncated_interval`: it locates L* by walking DOWN in RC-strand space (whose prefixes nest) and expanding the carried [lo, hi) interval outward with capped `shares_prefix` scans, instead of re-locating each candidate length from the model as the binary-search form does. Forward intervals do not nest, so the binary-search form pays a model launch per occ-query; the RC walk pays a few probes per step plus one forward `mem_search` at L*. Byte-identical to the binary-search form (same L*, same forward interval), verified by a proptest against both the independent `truncated_interval_oracle` and the binary-search twin. The `est_hint != 0` path delegates to the binary-search twin (which honors the `no_search` hint contract). The forward recovery uses a plain `mem_search`; the .isa-seeded fast path is a later, probe-count-only optimization. Swap `prmi_mem_search_backward_truncated_interval` to call the RC form (same signature, byte-identical output); the existing FFI test now exercises it.
nh13
force-pushed
the
feat/v0.3-trunc-iv-rc
branch
from
June 12, 2026 18:11
8aa920a to
f2e6bc6
Compare
nh13
added a commit
that referenced
this pull request
Jun 12, 2026
Add `forward_truncate_below_maximal`, the FORWARD analogue of the TRUNC_IV backward family (#27/#29/#30) for the reseed's right bound. Given a query and its already-found maximal match whose `occ < min_intv`, it finds the largest length L < match_len whose prefix occurs >= min_intv times and returns that interval (or the zero MemMatch when none qualifies). Forward PREFIX intervals nest directly (fixed start, shrinking end: query[..L] is a superset of query[..L+1]), unlike the LEFT reseed where prepending bases breaks forward nesting and forces the RC walk. So the walk carries [lo, hi) outward from the maximal interval as L shrinks — a short shares_prefix scan capped at min_intv per step, no model launch. `want_interval == false` returns only L* (the driver's next_pivot use); `true` recovers the exact interval by galloping find_boundary out from the capped walk's interior seed (seed-independent, so byte-identical to a cold mem_search). Adapted to main: 6-arg shares_prefix/ref_less (no keyed_compare_mask). Verified byte-identical against an independent per-length mem_search oracle across reference-lifted queries and a spread of min_intv. Pub primitive with no in-tree caller until the collect reseed module lands (the proptest is its only consumer).
nh13
added a commit
that referenced
this pull request
Jun 12, 2026
Add `forward_truncate_below_maximal`, the FORWARD analogue of the TRUNC_IV backward family (#27/#29/#30) for the reseed's right bound. Given a query and its already-found maximal match whose `occ < min_intv`, it finds the largest length L < match_len whose prefix occurs >= min_intv times and returns that interval (or the zero MemMatch when none qualifies). Forward PREFIX intervals nest directly (fixed start, shrinking end: query[..L] is a superset of query[..L+1]), unlike the LEFT reseed where prepending bases breaks forward nesting and forces the RC walk. So the walk carries [lo, hi) outward from the maximal interval as L shrinks — a short shares_prefix scan capped at min_intv per step, no model launch. `want_interval == false` returns only L* (the driver's next_pivot use); `true` recovers the exact interval by galloping find_boundary out from the capped walk's interior seed (seed-independent, so byte-identical to a cold mem_search). Adapted to main: 6-arg shares_prefix/ref_less (no keyed_compare_mask). Verified byte-identical against an independent per-length mem_search oracle across reference-lifted queries and a spread of min_intv. Pub primitive with no in-tree caller until the collect reseed module lands (the proptest is its only consumer).
nh13
added a commit
that referenced
this pull request
Jun 12, 2026
Add `forward_truncate_below_maximal`, the FORWARD analogue of the TRUNC_IV backward family (#27/#29/#30) for the reseed's right bound. Given a query and its already-found maximal match whose `occ < min_intv`, it finds the largest length L < match_len whose prefix occurs >= min_intv times and returns that interval (or the zero MemMatch when none qualifies). Forward PREFIX intervals nest directly (fixed start, shrinking end: query[..L] is a superset of query[..L+1]), unlike the LEFT reseed where prepending bases breaks forward nesting and forces the RC walk. So the walk carries [lo, hi) outward from the maximal interval as L shrinks — a short shares_prefix scan capped at min_intv per step, no model launch. `want_interval == false` returns only L* (the driver's next_pivot use); `true` recovers the exact interval by galloping find_boundary out from the capped walk's interior seed (seed-independent, so byte-identical to a cold mem_search). Adapted to main: 6-arg shares_prefix/ref_less (no keyed_compare_mask). Verified byte-identical against an independent per-length mem_search oracle across reference-lifted queries and a spread of min_intv. Pub primitive with no in-tree caller until the collect reseed module lands (the proptest is its only consumer).
nh13
added a commit
that referenced
this pull request
Jun 13, 2026
`mem_search_backward` allocated a fresh `Vec` for the reverse-complement query on every call (the consumer fires ~50 backward calls/read). Build it into a 256-byte stack buffer for the common read length instead, falling back to the heap only for reads longer than the buffer (e.g. long-read tech). No probe/result change — byte-identical (the `mem_search_backward_equals_maximal_backward_step` oracle proptest passes unchanged) — just removes the per-call heap allocation from the hot backward path. This is the exact `rc_stack`/`rc_heap` pattern already merged in `mem_search_backward_truncated_span_rc`/`_interval_rc` (#27/#30), applied to `mem_search_backward`'s RC build.
nh13
added a commit
that referenced
this pull request
Jun 13, 2026
`mem_search_backward` allocated a fresh `Vec` for the reverse-complement query on every call (the consumer fires ~50 backward calls/read). Build it into a 256-byte stack buffer for the common read length instead, falling back to the heap only for reads longer than the buffer (e.g. long-read tech). No probe/result change — byte-identical (the `mem_search_backward_equals_maximal_backward_step` oracle proptest passes unchanged) — just removes the per-call heap allocation from the hot backward path. This is the exact `rc_stack`/`rc_heap` pattern already merged in `mem_search_backward_truncated_span_rc`/`_interval_rc` (#27/#30), applied to `mem_search_backward`'s RC build.
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.
v0.3 perf series (authored fresh on
main; the fast twin of #29's binary-searchmem_search_backward_truncated_interval).What
Adds
mem_search_backward_truncated_interval_rcand swaps theprmi_mem_search_backward_truncated_intervalFFI to call it. Same contract as #29 — find the largest left spanLwithocc(L) >= min_intv(floored atanchor_len), return its exact forward interval — but it locatesL*by walking DOWN in RC-strand space instead of re-locating each candidate length from the model.Why it's faster
Forward intervals at different lengths do not nest, so the binary-search form pays a fresh model launch (the dominant reseed cost) per occ-query. RC-strand prefixes do nest:
Q[..L] = RC(read[anchor_end-L .. anchor_end]), soQ[..L]'s interval is a superset ofQ[..L+1]'s. Starting from the maximal extension's in-interval seed, each shorter length expands the carried[lo, hi)outward with a shortshares_prefixscan capped atmin_intv— no model launch.occis strand-symmetric (occ(RC(P)) == occ(P)), soL*is identical; one forwardmem_searchatL*recovers the returned interval.Byte-identity
Same
L*, samemem_search(window)as the binary-search twin. The new proptest cross-checks_rcagainst both the independenttruncated_interval_oracle(#29) and the binary-search_intervalit replaces in the FFI, acrossmin_intv ∈ {0,1,2,5,20,1e6}. The existing FFI test now exercises the_rcpath.Scope notes
mem_search; the.isa-seeded fast path (which would skip the one cold forward locate's model launch) is a later, probe-count-only optimization that needs the seed-window core — byte-identity is unaffected.est_hint != 0delegates to the binary-search twin (which honors theno_searchhint contract); the RC walk is the coldest_hint == 0reseed's lever, the path the consumer's live reseed takes.Self-review
Pre-PR
/coderabbitai-reviewflagged that theocc_count == 0boundary — reachable through the FFI but not from a found anchor — was asserted only by trace. I traced both forms to floor at the anchor (mem_search_backwardreturns{0,0,0}→span_max=0 <= anchor_len→anchor; copying the perf source verbatim would have returned{0,0,0}and diverged) and added an explicit cross-check assertion to lock the FFI-swap invariant at that branch.Summary by CodeRabbit
Documentation
Refactor