Skip to content

Fix prebuilt-index alignment, split-index output, and write failures - #34

Open
DylanCosto wants to merge 1 commit into
mainfrom
fix/review-correctness
Open

DylanCosto wants to merge 1 commit into
mainfrom
fix/review-correctness

Conversation

@DylanCosto

@DylanCosto DylanCosto commented Oct 29, 2026 •

Copy link
Copy Markdown
Owner

Prebuilt indexes now supply the k-mer size for derived chaining penalties as well as seeding. A k=15 index produces the same output as mapping directly with -k15 -w10, rather than silently retaining the preset's k=19 penalties and changing alignments.

Split-index merging now reads every query file, preserves input read groups and filters copied comments consistently. The single-part FASTA fast path also honors --split-prefix. Multi-part SAM/BAM output automatically uses the existing merge path, producing a complete reference dictionary instead of turning later-contig alignments into unmapped records or failing BAM conversion. Automatically generated temporary files are cleaned up on success and failure; --split-prefix still controls their location. Merging requires regular query files because they are reread; stdin/pipes are rejected with an actionable message.

SAM/PAF output is explicitly flushed before success is reported. Writer failures also disconnect the input queue before joining the reader, preventing a full-disk/broken-output error from hanging the process.

Validation on x86-64 Linux:

  • cargo test --release --locked --features jemalloc: 214 passed, 5 benchmark/stress tests ignored; existing golden output hashes unchanged. Re-ran all 13 CLI regression tests after final edits.
  • cargo clippy --release --locked --features jemalloc --all-targets passed.
  • All 13 Python validation tests and both archived-evidence integrity checks passed.
  • Focused coverage only: extend the existing index-equivalence regression for a mismatched preset k; add one split-output regression covering multiple queries, reference formats and BAM headers; add one Linux output-error regression covering final flush errors and cancellation without a hang.

No full-genome variant-calling benchmark or native ARM execution was run locally. Default matching-preset golden outputs are unchanged; the alignment change restores equivalence to direct FASTA mapping for indexes with a different k.

Performance benchmark — 2026-10-29

Compared main b44551d with this PR's fd23a9d on an i9-12900 under x86-64 WSL/Linux, Rust 1.99.1, release + jemalloc, warm-cache Linux ext4 input/output. Runs were sequential with alternating version order and one warmup. ONT input: 40,960 reads / 254.4 Mb (256 renamed copies of the bundled synthetic fixture). HiFi input: 8,192 reads / 112.4 Mb (1,024 renamed copies of the eight real HG002 fixture reads). References are deliberately small (0.66 Mb / 84 kb).

Case, 8 threads Main median PR median Measured wall-time change
Single-part ONT SAM, 3 paired trials 4.43 s 4.37 s -1.1%
Single-part ONT sorted BAM, 5-pair follow-up 5.08 s 5.33 s +5.0%
Single-part HiFi SAM, 5-pair follow-up 3.33 s 3.33 s -0.05%
Automatic multi-part SAM, 3 paired trials 7.03 s (invalid header) 33.87 s (valid) 4.82× as long
Explicit --split-prefix SAM, 3 paired trials 34.09 s 34.45 s +1.1%

Confirmed performance caveat: automatic multi-part SAM/BAM now selects the existing serial split-mapping/merge path. This fixes the invalid output but loses parallel mapping throughput. Peak RSS for multi-part SAM falls from about 1,100 MiB to 74 MiB. Existing explicit split mode has essentially the same cost before and after. Preserving multi-part parallel throughput needs a follow-up change to that mapping pass before merging if this regression is unacceptable.

The old automatic multi-part SAM has 25,088 records referencing contigs absent from its header; the PR has all three contigs declared and 40,960 distinct primary read names. Old automatic multi-part BAM fails with reference not in header; the PR succeeds in 34.78 s median, so there is no successful old BAM runtime to compare.

Ordinary SAM/HiFi and sorted-BAM logical outputs are byte-identical after removing @PG. The sorted-BAM timing is noisy: the initial three-pair pass showed +12.5%, while the five-pair follow-up showed +5.0% with overlapping ranges (main 4.83–5.33 s; PR 4.73–5.68 s). No precise ordinary-run speed guarantee follows from these small samples. Old/new raw index files are byte-identical; the follow-up uses the same index file for both executables.

This is a code-path throughput/output-equivalence benchmark, not a whole-genome throughput or variant-calling accuracy benchmark.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant