Skip to content

Batch PRs #166, #170, #171, #157, #159 - #200

Merged
Psy-Fer merged 12 commits into
mainfrom
batch/small-independent
Aug 5, 2026
Merged

Batch PRs #166, #170, #171, #157, #159#200
Psy-Fer merged 12 commits into
mainfrom
batch/small-independent

Conversation

@Psy-Fer

@Psy-Fer Psy-Fer commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Batch merge of five small, independent PRs from @BenjaminDEMAILLE. Each is preserved as its own commit(s) with original authorship; the batch is split by commit so it reads PR by PR.

Closes #166, #170, #171, #157, #159.

What's in it

PR Commits What
#166 a10b29b, 86ffe98 --runThreadN 1 ran on every logical core. The rayon pool was only configured above 1, and skipping build_global leaves rayon's default of one worker per core.
#170 164b297, 7d7b367 Drop Transcript::read_seq, a public field nothing reads — a full read copy per finalised alignment.
#171 a4d6226 test/bench_ab.sh: interleaved A/B harness that samples CPU idle before and after every run and drops rounds it could not measure honestly, rather than folding them into the median.
#157 d181ac3, 4f7b6fe, 1a343b2, caec836 --soloCellReadStats CB writes Solo.out/<feature>/CellReads.stats, plus --genomeChrSetMitochondrial.
#159 ae575c7 --runMode soloCellFiltering <raw dir> <output prefix> — cell-call an existing raw matrix without aligning.

Changes made while merging

None. All five applied cleanly and nothing needed correcting. The only manual step was a CHANGELOG.md conflict between #157 and #159, which both add an entry at the same anchor; both entries are kept, resolved into ae575c7.

Two things checked rather than assumed, since they are the claims that carry risk:

Validation

All five are output-neutral by construction — a thread-pool fix, a dead-field removal, a shell script, and two features reached only via new flags. Verified rather than assumed:

Byte-identical to current main on the default paths, records compared with headers excluded (the @PG CL: line necessarily differs by output prefix):

records differing from main
SE 10k yeast 0
PE 10k yeast 0
STARsolo, 10x mouse chr19 0

Solo Gene raw matrix.mtx, barcodes.tsv and features.tsv are all byte-identical to current main on the same run.

Against STAR 2.7.11b, unchanged from the recorded baselines:

  • SE 10k yeast: 8788/8926 (98.5%)
  • PE 10k yeast: 8390 both-mapped, 0 half-mapped, per-mate position agreement 16605/16780 (99.0%)

Gate: 593 tests pass, cargo clippy --all-targets clean, cargo fmt --check clean.

# SE
./target/release/rustar-aligner --runMode alignReads --genomeDir $DATA/indices_rustar \
  --readFilesIn $DATA/reads/ERR12389696_sub_1_10k.fastq.gz --readFilesCommand zcat \
  --outFileNamePrefix out_se_ --outSAMtype SAM --runThreadN 1
python3 test/compare_sam.py --rustar-aligner out_se_Aligned.out.sam --star $DATA/star_10k_/Aligned.out.sam

# PE
python3 test/compare_pe.py out_pe_Aligned.out.sam $DATA/star_10k_pe_/Aligned.out.sam

BenjaminDEMAILLE and others added 11 commits August 5, 2026 16:11
The rayon global pool was configured only when `--runThreadN` was
greater than 1. Skipping the build at 1 does not give one thread: it
leaves rayon's default, which is one worker per logical core. So
`--runThreadN 1` ran the whole machine.

Measured on 200k reads, before: 2.44 s wall at **1300% CPU**. After:
26.23 s wall at 100% CPU. The old figure was not a fast single-threaded
run, it was a sixteen-way run wearing the wrong flag.

This matters beyond the flag reading falsely. A scheduler or a container
given one CPU gets sixteen worker threads; on a shared machine the run
oversubscribes every other job; and with a thread-caching allocator each
of those threads keeps its own heap, which is the very cost the comment
above this code says the pool sizing exists to avoid.

It also means the project's thread-invariance checks were weaker than
they read: the `--runThreadN 1` leg was not a one-thread leg. Verified
now that it is one: records are byte-identical between 1 and 8 threads
on 200k real reads, and byte-identical to the previous binary's output
at `--runThreadN 1`. Only the `@PG` `CL:` line differs between thread
counts, because it records the command line.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The field was filled with a full copy of the read at every finalised
transcript and assigned again in four more places, and no code in the
crate reads it. Not "reads it rarely": the compiler was asked, and after
deleting the field every one of the 97 resulting errors is a struct
literal or an assignment. There is no read site, in `src` or in tests.

Measured on 200k real reads at 8 threads: about 50 transcripts are
finalised per read, so removing it takes **15.1 million allocations off a
200 million total** (7.6%) and 3.87 GB of copying.

Wall clock does not move, and that is worth recording rather than hiding:
six interleaved rounds at 87-92% CPU idle give medians 20.90 s against
20.65 s with the direction mixed, inside the run-to-run spread. mimalloc
is fast enough that seventy-five small allocations per read do not
surface. The reason to remove it is that it is dead weight, not that it
is slow.

That number also calibrates #168 downward: if removing 7.6% of the
allocations changes nothing measurable, the rest of the allocation
programme is unlikely to be worth a new dependency.

`Transcript::read_seq` is `pub`, so this is an API removal and needs
sign-off. Nothing outside the crate can be relying on its contents being
meaningful, though, since it is only ever written.

Output-neutral: SAM byte-identical on 200k real reads.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every perf measurement on this project needs the same three guards, and
I got each of them wrong at least once in a single session:

An earlier harness sampled CPU idle once, before the first run. Three
separate measurements then drifted below the threshold mid-series and it
kept going, leaving me to spot the contaminated rounds by eye in the
output. This samples idle before and after every run, drops a round if
any of the four samples falls short, and prints how many it dropped, so
a median over four surviving rounds cannot be mistaken for a median over
six.

The check is on CPU idle rather than load average, because load average
is an exponential average over minutes: it refused to measure at 2.24 on
a machine whose cores were all free.

It reports the spread within each side next to the difference between
the medians, and says so in as many words when the difference is
smaller. Two changes I measured looked like wins on medians alone and
were inside the spread.

Both sides run as ./rustar-aligner with --outFileNamePrefix ./ from
inside their own directory, because the @pg CL: line records argv
verbatim: running ./old against ./new is enough to make the output
differ, which cost me two false "output is not neutral" alarms.

The header documents the fourth trap, which no script can enforce:
timing a total hides the part that changed. BAM writing is 1-4% of a
yeast run, so a total dominated by alignment cannot resolve a change to
the writer. Run the None configuration alongside and read the
difference.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One row per cell barcode with the fifteen counters STARsolo reports: how the
barcode matched, whether the read mapped to one locus or several, whether it
landed on a feature, where in the gene and on which strand, whether it was
mitochondrial, and whether it reached the matrix. The per-cell UMI and gene
totals come from the raw matrix rather than from the read counters, so they
agree with what the matrix says by construction.

Reads whose barcode never resolved are summed into a single `CBnotInPasslist`
row instead of being dropped. That row is the reason the file is useful: it is
the difference between "these cells look thin" and "most of the input never
reached a cell at all".

The region columns split by strand — an antisense read counts under `exonicAS`
or `intronicAS`, never under `exonic` or `intronic`.

`--genomeChrSetMitochondrial` names the chromosomes behind the `mito` column.
Without it the column is zero throughout, which is honest: no chromosome was
declared mitochondrial.

D24 comes with it. STAR emits these rows by walking a libc++ `unordered_map`,
which at these sizes is the reverse of each barcode's first appearance. That is
reproduced, including across threads: the per-read accumulator merges in read
order, so a threaded run writes the same file as a serial one. It stops being
reproducible past the point where libc++ rehashes, since the order then depends
on the bucket count. The values never differ, only which line they sit on.
Recorded in docs-old/dev/divergences.md.
Section 3.2, in the format CONTRIBUTING.md asks for.
CONTRIBUTING.md requires the description to match the code; the entries for the
other themes split out of #152 belong to their own PRs.
Reads are folded in under a mutex, so there are no per-thread partials to merge;
the function was reachable only from its own test. CONTRIBUTING.md rules out
shipping a function no production path reaches, and the PR description claimed
its test as evidence of thread-safety that the mutex actually provides.
Cell-calls an existing raw count matrix without aligning anything, taking the
raw directory and an output prefix as STAR does:
`--runMode soloCellFiltering /path/to/raw/ /path/to/out/prefix`.

Cell calling is a decision about a matrix, not about reads. Re-calling with
different `--soloCellFilter` parameters should not mean re-aligning 400 million
reads, and a matrix produced by another tool should be callable too.

The matrix is streamed into the same temp-body form the align path builds, so
`called_cells` and `emptydrops_called` are the identical code here and there
rather than a second implementation free to drift. Counts are rounded on the way
in: a multimapper matrix carries real values, and the filters work on UMI
totals.

`--runMode` becomes a token list, because that is what STAR's is: the mode
followed by its arguments. The mode itself is now validated rather than falling
back to `alignReads`, so a typo is refused instead of quietly running something
else.

The standalone `emptydrops` binary still exists and still carries its own copy
of the algorithm, which no longer matches this one. Removing it means moving
`test/solo_genefull_compare.py` and `test/solo_genefull_h5_compare.py` to the
new mode first, so it is left alone here rather than broken.
@Psy-Fer
Psy-Fer enabled auto-merge (squash) August 5, 2026 06:33
@Psy-Fer
Psy-Fer merged commit 0ca3803 into main Aug 5, 2026
10 checks passed
@Psy-Fer
Psy-Fer deleted the batch/small-independent branch August 5, 2026 07:11
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.

2 participants