Repository navigation
Fix prebuilt-index alignment, split-index output, and write failures - #34
Open
DylanCosto wants to merge 1 commit into
Open
DylanCosto wants to merge 1 commit into
DylanCosto wants to merge 1 commit into
Conversation
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
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.
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-prefixstill 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-targetspassed.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
b44551dwith this PR'sfd23a9don 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).--split-prefixSAM, 3 paired trialsConfirmed 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.