8000 feat(index): default SA sample rate to 1/8 (-u 3) by nh13 · Pull Request #38 · nh13/minibwa · GitHub
[go: up one dir, main page]
Skip to content

feat(index): default SA sample rate to 1/8 (-u 3) - #38

Draft
nh13 wants to merge 2 commits into
masterfrom
sa-default-u3
Draft

feat(index): default SA sample rate to 1/8 (-u 3)#38
nh13 wants to merge 2 commits into
masterfrom
sa-default-u3

Conversation

@nh13
@nh13 nh13 commented Sep 3, 2026 •
edited by coderabbitai Bot
Loading
Copy link
Copy Markdown
Owner

What

Changes the default suffix-array sample rate from 1/16 (-u 4) to 1/8 (-u 3) when building an index, and documents the tradeoff.

Why

A denser suffix array means fewer BWT walk steps to resolve each SA lookup, so mapping runs measurably faster out of the box. Measured ~1.12× wall-clock on WGS-5M at -t 16. The cost is index memory: the SA roughly doubles in size, taking the hg38 index from ~9 GB to ~12 GB. Anyone who needs the smaller footprint can still pass -u 4 (or higher) explicitly — this only moves the default.

Testing

  • Builds clean; index/map round-trip unchanged apart from the denser SA.
  • README callout updated with the measured speedup and the corrected memory figures.

Commits

  • feat(index): default SA sample rate to 1/8 (-u 3)
  • docs: note the 1/8 default SA sampling and the -u 4 escape hatch

Summary by CodeRabbit

  • New Features

    • Indexing now uses denser suffix-array sampling by default, improving mapping speed by approximately 12% on hg38.
    • Added guidance for adjusting sampling density, including how to restore the previous default.

  • Documentation

    • Documented the performance and memory trade-offs of different suffix-array sampling settings.

8000
nh13 added 2 commits August 19, 2026 17:10
@nh13
feat(index): default SA sample rate to 1/8 (-u 3)
8f2b1eb 
The 1/16->1/8 step maps ~7% faster (measured end-to-end on hg38 PE WGS)
for ~+3 GB index/RAM. The rate is stored per-index in the .mbw header, so
existing 1/16 indexes keep mapping; only new builds change. -u 4 restores
the previous 1/16 sampling.
@nh13
docs: note the 1/8 default SA sampling and the -u 4 escape hatch
b605368
@nh13
nh13 commented Sep 4, 2026
Copy link
Copy Markdown
Owner Author

@coderabbitai pause

@nh13
nh13 marked this pull request as draft September 4, 2026 00:30
@nh13 nh13 added feature review vehicle for a manifest feature; never a merge vehicle uninstalled No features.toml block yet; not installed into the distribution (dist). Remove when installed. labels Sep 4, 2026
@nh13
nh13 commented Sep 4, 2026
Copy link
Copy Markdown
Owner Author

@coderabbitai review

@nh13
nh13 commented Sep 4, 2026
Copy link
Copy Markdown
Owner Author

@coderabbitai full review

1 similar comment
@nh13
nh13 commented Sep 4, 2026
Copy link
Copy Markdown
Owner Author

@coderabbitai full review

@coderabbitai
coderabbitai Bot commented Sep 4, 2026 •
edited
Loading
Copy link
Copy Markdown
✅ Action performed

Full review finished.

@coderabbitai
coderabbitai Bot commented Sep 4, 2026 •
edited
8000 Loading
Copy link
Copy Markdown

Note

Reviews paused

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Team

Run ID: cfada3cc-f023-49c1-b2ab-a74e03bf5c5c

📥 Commits

Reviewing files that changed from the base of the PR and between f0e1174 and b605368.

📒 Files selected for processing (2)
  • README.md
  • index.c

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Walkthrough

The default suffix-array sampling changes from 1/16 to 1/8 for all indexing commands. The README documents the new default, performance and memory measurements, and available sampling overrides.

Changes

Suffix-array sampling defaults

Layer / File(s) Summary
Update sampling defaults and documentation
index.c, README.md		 main_genbwt, main_gensa, and main_index now default to sa_bit 3. The README documents the 1/8 sample rate, measured resource changes, and -u alternatives.

Estimated code review effort: 2 (Simple) | ~5 minutes

Merge Risk: ⚪ Minimal · up to b6053

New index builds use denser 1/8 suffix-array sampling for faster mapping with higher index memory use; users can retain the prior 1/16 setting with -u 4. The implementation and documentation are aligned, with no current merge-blocking risk identified.

Suggested reviewers: lh3

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: the default suffix-array sample rate changes to 1/8 with the -u 3 setting.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sa-default-u3

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@nh13 nh13 removed the uninstalled No features.toml block yet; not installed into the distribution (dist). Remove when installed. label Sep 5, 2026
@nh13
nh13 commented Sep 7, 2026
Copy link
Copy Markdown
Owner Author

@coderabbitai pause

@coderabbitai
coderabbitai Bot commented Sep 7, 2026
Copy link
Copy Markdown
✅ Action performed

Reviews paused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

feature review vehicle for a manifest feature; never a merge vehicle

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@nh13
0