test(spectrum): harden reseed-neighbor-scan (boundary cases) + collect_wall bench - #49
Conversation
…ts corpus Replace the O(lmax-L*) per-length walk in forward_truncate_below_maximal with an O(min_intv) neighbor-LCP scan: read the LCP of the query with the SA suffixes just outside the interval and extend the larger-LCP side until occ>=min_intv, then recover the exact interval via find_boundary (span-only for want_interval=false). Byte-identical to the old walk / naive oracle, proven by the existing forward_truncate_below_maximal_equals_oracle proptest plus a new repeats/ties corpus test exercising shared-L* ties and occ(maximal) in [2,min_intv), both want_interval modes. Probe budget on long-unique reads: 264 -> 18.
Adversarial review of the reseed-neighbor-scan flagged three reasoned-correct but unexecuted branches. Add forward_truncate_boundary_cases covering: (a) lmax==1 (lcap=0 -> zero), and (b) SA-end exhaustion where min_intv exceeds any achievable occ, so the scan walks both neighbours off the SA ends and breaks on llo==0 && lhi==0 -> zero (not via the lmax cap). Both modes (want_interval true/false) and an oracle cross-check. No production-code change.
|
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)
WalkthroughReplaces two O(lmax−L*) walk-down loops in ChangesNeighbor-LCP scan rewrites in spectrum.rs
Wall-clock benchmark harness
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 resume |
✅ Action performedReviews resumed. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@prmi/examples/collect_wall.rs`:
- Around line 30-35: The envu function currently treats unparseable environment
variables the same as unset variables by using and_then with unwrap_or, silently
falling back to defaults instead of alerting users to invalid input. Modify the
envu function to distinguish between missing and invalid environment variables:
if std::env::var returns Ok but the parse fails, print an error message to
stderr and exit the process with a non-zero code, otherwise if the variable is
unset return the default value, and if it parses successfully return the parsed
value.
- Around line 53-56: The FASTQ parsing loop starting at lines 53-56 is silently
discarding errors and stopping when records are malformed or truncated, which
allows benchmarking incomplete data. Fix this by validating that all 4 required
FASTQ lines per record (header, sequence, plus line, and quality) are
successfully read: replace the current pattern of using let Some(Ok(...)) else {
break } and let _ = lines.next() with explicit expect() calls on each
lines.next() call so that any missing or error lines will cause the program to
fail immediately rather than continuing with partial records. This ensures the
loop fails closed on any malformed input or read errors.
🪄 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: 5d3df54e-b0dd-4ae8-b89f-3658c6d35152
📒 Files selected for processing (2)
prmi/examples/collect_wall.rsprmi/src/index/spectrum.rs
b078d78 to
7e292b1
Compare
|
@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 `@prmi/examples/collect_wall.rs`:
- Around line 47-48: Add validation after reading the pac file to ensure its
length matches the expected size for the given l_pac value. After the line where
pac is read using std::fs::read with the PRMI_PAC path, insert an assertion or
validation check that pac.len() equals (l_pac + 3) / 4. This validation must
occur before creating the PacEncoding::Packed struct to prevent panics or silent
data corruption from truncated or oversized pac files.
🪄 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: 4f692912-d523-4900-ad98-8f187cd7acba
📒 Files selected for processing (1)
prmi/examples/collect_wall.rs
Times the full per-read SMEM driver (ns/read) over a FASTQ; built at two commits (pre/post #48) it measures the reseed-neighbor-scan wall win on a large ref.
7e292b1 to
7aab436
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Stacked on #48 (perf/reseed-neighbor-scan). Tests + benchmarking only — no production-code change.
What
forward_truncate_boundary_cases). An adversarial review of the neighbor-LCP scan flagged three reasoned-correct but unexecuted branches; the random corpus is statistically unlikely to hit them. This locks them:lmax == 1→lcap = 0→ zero (nothing to truncate below a length-1 maximal).min_intvlarger than any achievable occ, so the scan walks both neighbours off the SA ends and breaks onllo == 0 && lhi == 0→ zero (the genuine exhaustion path, not thelmaxcap).want_intervalmodes, plus an oracle cross-check.collect_wall— a per-readcollect_smemswall-clock harness (ns/read), so the neighbor-scan win can be measured at two commits on a large reference.Why / evidence
Built at this base vs the pre-scan commit and measured on a Graviton c8g (128 Mbp synthetic reference, 3.49 GB suffix array):
The adversarial review verdict on the scan itself: byte-identity-sound, nothing blocks merge. This PR just closes the boundary-test gaps and adds the measurement harness.
Summary by CodeRabbit
Release Notes
New Features
collect_smems, with configurable runtime parameters via environment variables.Improvements
Bug Fixes / Tests