Repository navigation
Window bins capped by the index's genomeChrBinNbits; chimeric place() cannot hang - #361
Open
BenjaminDEMAILLE wants to merge 2 commits into
Open
BenjaminDEMAILLE wants to merge 2 commits into
BenjaminDEMAILLE wants to merge 2 commits into
Conversation
redefine_window_params skipped the genomeChrBinNbits cap when both alignIntronMax and alignMatesGapMax were 0, and read the command-line value rather than the index's. Genomes of many small contigs lost reads and reported repeats as unique. The cap now always applies with the index value, and the bin size uses STAR's integer divisions. place() cast a negative block length to usize, which stalled the run on one read. Such a candidate is now rejected. STAR stops with a bus error on the same read (DIVERGENCE.md 1.9, needs maintainer sign-off). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…shes The guard also rejected a trailing block of length exactly 0, which STAR keeps (its jRmax clamps to 0 and the new length is 0, not negative). That only mattered with --chimJunctionOverhangMin 0 or on the multimap path with min_score <= 0. Only the case whose length would wrap is rejected now, and DIVERGENCE.md 1.9 cites STAR's source for it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
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.
Part of #354 (findings 1 and 13).
Claims about STAR below are observed output of Homebrew STAR 2.7.11b; source not read. Each tool used its own index.
1. Window bins on genomes with many small contigs (finding 1)
Problem
redefine_window_paramshad two faults:genomeChrBinNbitscap when--alignIntronMaxand--alignMatesGapMaxwere both 0, which is the default.--genomeChrBinNbitsis normally given only togenomeGenerate, so at mapping time the cap saw the default 18.loaded_genome_paramsinsrc/index/mod.rsreads the index's value, but only into a local copy.So an index built with small chromosome bins kept 2^16 window bins. Contigs then shared a bin: reads were lost as "too short", and repeats were reported as unique.
What STAR does
The comment "STAR: no redefinition when both are 0" is half true, and I removed it.
Log.outon the 400-contig index:Log.outwinBinNbitswinBinNbits=16 > pGe.gChrBinNbits=11 redefining:--genomeChrBinNbits 18at mapping (index has 11)--winBinNbits 5--alignIntronMax 100000redefined winBinNbits=15, then capped;winFlankNbins=49 and winAnchorDistNbins=98--alignIntronMax 1450redefined winBinNbits=8;winFlankNbins=6 and winAnchorDistNbins=12--alignMatesGapMax 1450winBinNbits=8;winFlankNbins=6 and winAnchorDistNbins=12--alignIntronMax 1000, nGenome 14,450,688, index bins 12winBinNbits=8;winFlankNbins=4 and winAnchorDistNbins=8So with both limits 0, only the cap applies;
winFlankNbinsandwinAnchorDistNbinsstay as given. The last two rows also show integer divisions: 1450/4 and 14450688/40000 computed as floats would round to 9.mainused floats, so I changed that too. It is in the same formula and is one more reason the bins differed from STAR.STAR's own line
alignIntronMax=alignMatesGapMax=0, the max intron size will be approximately determined by (2^winBinNbits)*winAnchorDistNbins=589824is printed before the cap, so it can state 589824 when the real figure is 18432.Change
redefine_window_paramsalways applies the cap, and flanks are derived after it, only when a limit is set.run_alignsetsgenome_chr_bin_nbitsfrom the index before calling it, usingloaded_genome_params, nowpub(crate). This is the only caller.win_bin_window_disthad no caller; the log line now uses it. I removed its comment, which described uses that do not exist.src/align/read_align.rsandtests/alignment_features.rssaidwinBinNbits*winAnchorDistNbins (~589 kb). It is 2^winBinNbits, and the cap can lower it.Measured against STAR
400-contig fixture,
--genomeChrBinNbits 11, 4005 single-end reads. "Differing" is the count of lines incomm -3of sorted SAM fields 1-9 against STAR.main--alignIntronMax 100000main--alignIntronMax 1450main--alignMatesGapMax 1450main--winBinNbits 5mainThe target numbers are reached. The rounding change does not move any count on this fixture; it is only checked through
winBinNbitsin the table above, which is whatwindow_params_follow_star_logasserts.2. Chimeric detection hang (finding 13)
Problem
In
place(),new_lwas computed asi64and cast tousize. When the trailing segment's first block ends before the leading segment's last block, the value is negative and becomes a huge length. Loops over the block length then do not finish in any useful time.Reproduced on
mainwith the audit's read pair (a 153-base mate 1 and a 12-base mate 2) on its 29 kb genome,--chimSegmentMin 10 --chimMultimapNmax 20 --chimScoreDropMax 200 --chimNonchimScoreDropMin 0 --chimScoreSeparation 0, undertimeout:--chimJunctionOverhangMinmainChimeric.out.junctioncounts exclude its header and comment lines.Change
place()returnsNoneonly when the trailing block's first block ends before the leading segment's last block starts (roStart1 + L <= roStart0), the case whose new length wraps in STAR'suint(ReadAlign_chimericDetectionOld.cpp:154-155,:253-256). A block ending exactly there, orjr_best == jr_maxbecause the scan skips offsetreadLength[0], gives length 0, which STAR keeps; so doesplace()since74d8a02(the first version rejected it too, which review against STAR's source caught). The overhang filters decide it later, so this only matters with--chimJunctionOverhangMin 0.Measured against STAR
Random multi-segment reads on the same genome, with repeats and canonical introns: 4000 SE, 4000 PE of which about 30% have a 12-base mate 2.
Chimeric.out.junctionlines, sorted:main--chimMultimapNmax 20, overhang 1--chimMultimapNmax 20, overhang 1--chimMultimapNmax 20, overhang 2--chimMultimapNmax 20, overhang 3--chimMultimapNmax 20, default overhangDeliberate divergence, needs maintainer sign-off
DIVERGENCE.md§1.9: where STAR dies with a bus error, this skips the candidate and exits 0. There is no STAR output to match in that case, so the 9576 lines above cannot be checked against STAR.Tests
Each failed when I took out the corresponding fix, and passes with it:
src/params/mod.rs:window_params_follow_star_log, with the expected values from the STARLog.outtable above.tests/alignment_features.rs:test_window_bins_capped_by_the_index_chr_bins, 200 contigs, index built with--genomeChrBinNbits 11, not repeated at mapping. Expected: 800 unique, 1 multi with 4 records. STAR gives those numbers on the same data, with 0 differing lines in SAM fields 1-9;maingives 801 and 0, with 1 record.src/chimeric/detect.rs:place_rejects_a_trailing_block_ending_before_the_last_leading_block,place_keeps_a_junction_leaving_no_trailing_bases.cargo test --release,cargo clippy --all-targetsandcargo fmt --checkare clean on macOS.Not checked
test/parity_suite.shwas not run.CONTRIBUTING.mdwere not run. Their index uses the default 18-bit bins and their runs set no intron or gap limit, so by reading the code I expect no change, but that is not measured.--chimOutType WithinBAMwas not compared on the reads that hang.place()directly and assertNone.🤖 Generated with Claude Code