Repository navigation
Read loops: one shared per-read output and writer stage - #366
Open
BenjaminDEMAILLE wants to merge 1 commit into
Open
BenjaminDEMAILLE wants to merge 1 commit into
BenjaminDEMAILLE wants to merge 1 commit into
Conversation
The single-end, paired-end, STARsolo and STARsolo paired loops were near copies. They now share the record gate (OutputRules), read and pair statistics, pair records, the decoder thread, the WASP and chimeric set-up and the output name. The solo loops share their consumer and tags; single-end and paired-end share run_writer_stage. Behaviour is unchanged, differences between the loops included. 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 (finding 19).
Stacked on #365 (
chore/dead-code), itself stacked on #346. Merge after those.Problem
src/lib.rshas four read loops (single-end, paired-end, STARsolo, STARsolo paired) of 450 to 760 lines each, which are near copies. A rule added to one did not reach the others.Change
Plain functions in
src/lib.rs, before the loops. No trait, no framework, one shared stage.OutputRules::build_records: which records a read may produce (--outSAMtype None,--outSAMunmapped, multimap limit)spawn_decoder,read_limit,output_read_nameapply_rg_per_file_batch, now generic over the resultrecord_single_statsPairOutcome,record_pair_stats,build_aligned_pair_records(with a hook for what only the loop knows: WASP and splicing tags, or gene tags)run_writer_stage: chimeric, held reads, signal, SAM/BAM, transcriptome, unmapped, BySJout second stage, signal tracks, trailer, flushesopen_chimeric_writer,load_wasp_contextSoloReadProduct,SoloRules,consume_solo_products,solo_tag_out,add_gene_tags_in_output_order,add_solo_read_tagsalign_reads_single_endalign_reads_soloalign_reads_solo_pealign_reads_paired_endsrc/lib.rsgoes from 4319 to 4190 lines (43 of them a new test), so the gain is modest in total lines: the loops lose 831, the shared stage gains 640.Still per loop. The solo loops keep their consumer on the dispatcher thread: moving them to a writer thread would change threading, so they do not use
run_writer_stage. The clip-to-empty early returns also remain in each loop, because they differ (below); they readrules.output_unmapped.Hot path. No new per-read allocation:
build_recordstakes closures, and the gate pushes into the same buffer.PairOutcome::both_mappedis theVecthat was already built per pair, now of&PairedAlignment, and the PE loop no longer builds a second one for quant.AlignmentBatchResultsandSoloReadProductkeep their fields, sobatch_size_forgives the same batch sizes.One difference, on stderr only: PE now logs
Chimeric junction output completeat info level, as SE did. Seen in a diff of the two stderr logs of a chimeric PE run; it was the only line that differs in that run.Differences between the loops that look like bugs (kept, not fixed here)
Read from the code; none of them changes in this PR.
build_unmapped_record(&read.sequence…)), but use the clipped sequence and quality in solo and solo paired.--outSAMunmapped Within.clip_mate) unless CellRanger4; solo paired has no CellRanger4 path and uses fixed clips only.bedGraph(finding 4 of Tracking: fixes from the whole-repo audit (main at 32fbbc1) #354).results.len()towards the multimap limit, wheresignal_contriband SAM output countboth_mapped.WithinBAM, PE records a chimeric read without checkingchim_segment_min, SE checks it. Probably harmless, as chimeras only exist when it is set.u32, in both solo loops: it wraps past 4 294 967 295 reads.Measured
macOS, aarch64, 16 cores. Baseline: clean build of
aeba641(before the dead-code PR). Also compared with the dead-code PR'sbcddd31. Comparison: this branch.Output. 84 scenarios, each at
--runThreadN1, 4 and 8, every binary under the sameargv[0]and relative prefix so that command lines in headers match. md5 of every output file, raw BAM and SAM included, plus decoded BAM records andLog.final.outwithout its clock lines.Log.out,Log.progress.outand stderr are not in the md5s.The 4 rows are
Solo.out/{Gene,GeneFull}/CellReads.statsat 4 and 8 threads in one scenario. This is existing behaviour: two runs of the baseline differ from each other in that file, and baseline, baseline and this branch have the same md5 once sorted (64136fbd), so only the row order changes between runs. At 1 thread it is identical. Not touched here.Scenarios cover all four loops: SAM, BAM unsorted and sorted,
--outStd,Within, TranscriptomeSAM, GeneCounts,Fastx, chimeric junctions,WithinBAM, BySJout, fixed clip, clip to empty, adapter, CellRanger4,bedGraph, WASP, splicing tag, per-file RG, NoQS,None, 2-pass, read limits, multimap limits 0 and 1, half-mapped pairs, STARsolo Gene, GeneFull, SJ, Velocyto, EM,CB_samTagOut, a third barcode file and--soloBarcodeMate 1.The fixture was checked to reach these paths on the baseline: SE 15 965 unique, 1614 multi, 1417 too short, 1003 other, of 20 000 reads; a second fixture gives 2151 (SE) and 2152 (PE) chimeric reads; relaxed
--outFilter*OverLreadgives 92 records of half-mapped pairs in PE and in solo paired.Scenarios the program refuses:
sp_tag_m(an invalid parameter combination of mine) and 5 of the 9 earlier scenarios (input files the reader rejects). Same exit code 1 in all binaries; the error text is the same for the 3 I compared, which exercises the decoder's error path.Decoded records and every non-header file are also identical across 1, 4 and 8 threads, apart from the same
CellReads.statsrows.Speed.
--runThreadN 8. The machine was not idle: unrelatedbwa-mem2andsalmonjobs were running, so wall time is noisy (the same binary varies between 4 and 16 s). Instructions retired and CPU time are the comparison; wall time is given as the minimum of 5. From/usr/bin/time -l, binaries alternated.B200.fq, 801 000 reads, 176 MBInstructions are the median of 3 runs for SE and solo, 1 run for PE and solo paired. No measurable change in instructions; the CPU and wall differences are within the noise of this loaded machine. Median peak RSS, dead-code PR then this PR: 0.069 and 0.069 GB (
B200.fq), 0.053 and 0.056 (SE), 0.063 and 0.064 (PE), 0.098 and 0.090 (solo), 0.109 and 0.119 (solo paired). Thread timing makes it vary; the solo figures of the baseline are 0.104 and 0.105.Tests
cargo test --release: 816 tests pass,tests/determinism.rsandtests/phase9_threading.rsincluded. New:output_rules_tests::records_follow_the_output_rulesfor the gate.cargo clippy --all-targetshas no warnings;cargo fmt --checkpasses.Not checked
test/parity_suite.sh, and did not compare against STAR here; the claim is "same output as before".Log.outcontent.git merge-treereports conflicts inCHANGELOG.md,src/io/sam.rsandsrc/lib.rs, as the dead-code PR describes. I did not carry out the resolution.🤖 Generated with Claude Code