solo: cbMinP posterior threshold, oneExact guard, adapter-anchored geometry (replaces #150) - #165
Conversation
…uard `resolve_multi_cb` computed the count+quality posterior correctly, computed the normalising total, and then ignored it: any candidate with positive weight won by argmax. Two of STAR's conditions were missing. `cbMinP`. STAR requires the winner to hold at least 97.5% of the total posterior mass. Without it a barcode is assigned whenever any candidate has positive weight, including when two candidates are nearly tied — which is precisely the case the threshold exists to reject. A read whose barcode cannot be singled out should be discarded, not guessed. `oneExact`. Unless pseudocounts are in use, the winner must have been observed as an exact match at least once. A candidate whose only support is a pseudocount is eligible, not evidence. **This changes default counting behaviour**, and the existing unit test had to change with it: it asserted that priors of 10 against 3 resolve to the higher one, which is a 77% posterior share and now correctly resolves to nothing. The test was encoding the missing threshold. It now covers both sides — a decisive 1000-against-3 prior is accepted, a tie under pseudocounts is not. Verification I could not do here, and would want before this merges: the solo differential against real STARsolo (`test/solo_diff_docker.sh`). Reads that previously landed on an ambiguous barcode now land nowhere, so raw matrix counts will move, and the direction should be confirmed against the oracle rather than argued from the source. Everything in-tree is green — 560 lib + 26 integration — but that is not the same thing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…er-UMI rule `--soloOutFormatFeaturesGeneField3` sets the third column of `features.tsv`, STAR's default being `Gene Expression`. The sentinel `-` drops the column, giving the two-column form some downstream tools expect. Previously the string was hard-coded. Also adds `umi_valid_rejects_every_homopolymer`. No code change was needed — `is_homopolymer` already rejects a UMI of any single repeated base — but the rule deserves a test that says why it is deliberate. STAR has a quirk (`umiL = 0`) under which a non-A homopolymer UMI slips through its filter, so poly-C, poly-G and poly-T survive there and are counted. That is a defect rather than a rule: a UMI of one repeated base carries no information whichever base it happens to be. This diverges from 2.7.11b knowingly, and the test now records that instead of leaving it to be rediscovered as a difference in a differential run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… String `--soloChemistry` sets the CB/UMI geometry from a 10x chemistry name so it does not have to be spelled out four flags at a time: SC3Pv1 (14-base CB, 10-base UMI), SC3Pv2 (16/10), SC3Pv3, SC3Pv4 and SC5P (16/12), plus the CellRanger aliases. Unknown names are refused. The preset wins over an explicit `--soloCBstart` and friends, and warns when it overrode one. Half-applying a preset — taking its CB length but the flag's UMI start, say — would produce a geometry neither the user nor the preset asked for, and would fail silently in the counts rather than loudly at startup. `--soloCBtype` is declared and accepts `Sequence`. `String` is refused rather than accepted and ignored: barcodes here are 2-bit packed ACGT, and a non-ACGT barcode packed that way is nonsense rather than an approximation. Supporting it means a different whitelist representation throughout, which is its own change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`--soloCBposition` and `--soloUMIposition` take four fields, `startAnchor_startDist_endAnchor_endDist`, where the anchor codes are 0 = read start, 1 = read end, 2 = adapter start, 3 = adapter end. Only code 0 was accepted; the rest were a parse error. Codes 2 and 3 are the reason this needs a new layout rather than a wider parser: the adapter moves from read to read, so the offsets cannot be resolved once at startup the way every other layout's are. `SoloBarcodeLayout::Anchored` carries the specs with their anchors and the adapter, and resolves per read. Adds `--soloAdapterSequence` and `--soloAdapterMismatchesNmax`. The adapter search takes the leftmost position within the mismatch budget, which is what STAR takes — the first acceptable match, not the best-scoring one. A read where the adapter is not found, or where an anchor resolves outside the read, yields no barcode. Guessing would be worse than declining: a barcode read at an unknown offset is not salvageable, and a wrong barcode is a read attributed to the wrong cell rather than a read lost. Tests cover the adapter moving (both fields follow it), one mismatch inside the budget, no adapter at all, an anchor running off the front of the read, and the leftmost-match rule. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eftmost STAR's `localAlignHammingDist` (`SequenceFuns.cpp:341`) scores every offset and keeps the one with the fewest mismatches. This took the first offset that fit the mismatch budget, which is the opposite rule: an early approximate match beat a later exact one. Since `CB_UMI_Complex` measures its barcode positions from the adapter, choosing the wrong offset shifts the whole barcode, so this decides what the read is counted as, not merely where a match is reported. Reported by @Psy-Fer on scverse#150, with the upstream source. The tests did not catch it because their fixtures had a single candidate or an exact one, where leftmost and best-scoring give the same answer. There is now a test where they differ. Two further details from the same function, both of which were missing: Ties go to the leftmost of the best, because STAR replaces its incumbent only on a strictly smaller distance. An `N` in the adapter never counts as a mismatch. It is a wildcard in the query, not a base that fails to match, and treating it as a mismatch spends budget that STAR does not spend. STAR also seeds its best distance with the adapter length, so an offset that mismatches everywhere can never win. That only shows when the budget is as large as the adapter, where it keeps the answer `None` instead of an arbitrary offset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`--soloChemistry` does not exist in STAR 2.7.11b; it was carried over from STAR-rs. Adding it here widens the CLI beyond STAR's, which is a positioning decision for this project rather than something to arrive inside a PR about the barcode posterior. Raised by @Psy-Fer on scverse#150. The geometry it set is still reachable through the STAR flags it was a shorthand for: --soloCBstart, --soloCBlen, --soloUMIstart, --soloUMIlen. `--soloCBtype String` stays refused, as before: it is a real STAR parameter and refusing an unimplemented value is the honest answer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rustar-aligner has always rejected poly-A/C/G/T UMIs in every geometry. STAR rejects only poly-A under CB_UMI_Complex, because the four packed constants are built in the SoloReadBarcode constructor from pSolo.umiL while that path does not learn its UMI length until it has extracted one (SoloReadBarcode.cpp:16-21 against getCBandUMI.cpp:353-354), leaving all four constants zero. Poly-A packs to zero and is still caught; the other three are not. CONTRIBUTING requires a deliberate divergence to be recorded with the STAR source it departs from, and this one was not. Verified against the 2.7.11b source rather than asserted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… tests Both rules were asserted inside resolve_multi_prefers_higher_prior, whose name describes only the ordering. The threshold decides between correcting a barcode and dropping the read, and oneExact is not implied by it (a sole candidate holds the whole posterior and is still refused), so each gets a test that says what it checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
33c5e7d to
e77ac0a
Compare
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The two inert flags in #172 turn out to be fixed by two separate PRs, and together they close almost the whole gap. AttributionSame 20 000-read 10x fixture, STAR 2.7.11b against each build, raw
The pseudocounts were already wired on #165 also fixes the baseline: at default flags it takes rustar from 21 entries STAR does not have down to zero, 14 773 / 14 787 identical.
Both together
Full CellRanger-matching flag set, so this is the configuration the flags exist for. What is left37 entries of 13 708: 28 STAR-only, 2 rustar-only, 7 present in both and differing. One known cause, already stated on #173: STAR requires the winning gene of a multi-gene UMI to hold the top count among uncorrected UMIs as well ( The 28 STAR-only entries are the shape that condition would produce — STAR counting a UMI that we drop — so I would look there first. Note on merge orderNeither PR depends on the other, but they touch |
The figure I replaced this with was measured without scverse#165, whose cbMinP posterior threshold is a precondition for it. With scverse#165 the five flags move the matrix from 8.96% above CellRanger to 0.03% above it, which is what the original text said. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(solo): MultiGeneUMI_CR gives a tied UMI to nobody, not to everybody
`--soloUMIfiltering MultiGeneUMI_CR` kept every gene tied at the highest
read count. CellRanger's rule is the opposite on exactly that case: the
gene with the *strictly* highest count takes the UMI, and a tie means no
gene counts it.
STAR walks the genes keeping a running maximum and clears its winner
whenever it meets an equal count
(`SoloFeature_collapseUMIall.cpp:212-224`):
if (ig.second>maxu) { maxu=ig.second; maxg=ig.first; }
else if (ig.second==maxu) { maxg=-1; };
...
if ( maxg+1==0 ) continue; // not counted for any gene
One read per gene is the ordinary shape of a multi-gene UMI, and it is
always a tie, so the old rule made the flag inert in practice rather
than merely inaccurate. Measured on a 20 000-read 10x fixture (200 cells
from the real v3 whitelist, 400 genes, 720 UMIs deliberately shared
between two genes), against STAR 2.7.11b with the same flags:
identical entries STAR counts rustar counts
before 13 749 / 14 806 15 423 16 465
after 13 902 / 13 967 15 423 15 414
The flag removed nothing at all before; STAR removes 1 030 counts. The
gap goes from +1 042 to -9.
The outcome does not depend on the order the genes are visited — a
strict maximum always ends as the winner, a tie always ends with none —
so iterating a `HashMap` here stays deterministic.
`multi_gene_umi_cr_drops_a_tie_entirely` pins the case the old tests
missed: they only covered 3 reads against 1, where both rules agree.
Not yet implemented, and stated so rather than left to be discovered:
STAR applies a second condition, that the winning gene must also hold
the top count among *uncorrected* UMIs (`umiGeneMapCount0`, same file,
lines 226-232). That needs the pre-correction counts, which this code
does not keep. The 65 entries still differing out of 13 967 are the
place to look for its effect.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(changelog): record the MultiGeneUMI_CR tie fix
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(solo): MultiGeneUMI_CR decides ownership on corrected UMIs
STAR corrects UMIs within each gene *before* deciding which gene owns a
UMI, and applies two conditions, not one
(`SoloFeature_collapseUMIall.cpp:134-148` and `:203-235`):
1. one gene must hold a strictly higher read count than every other, on
the **corrected** UMI map — that is #173, already landed;
2. and that winner must not be beaten in the **uncorrected** map at the
same key.
The second condition exists because correction moves reads between UMIs:
a gene can win only because correction folded a neighbouring UMI onto it,
and STAR rejects that win rather than counting it.
Reproducing it needs the order STAR uses. The generic path here filters
multi-gene UMIs first and corrects afterwards, which cannot express either
condition: by the time correction happens the ownership decision is
already made. `MultiGeneUMI_CR` therefore takes its own path, which is
also what STAR does — the flag is only valid with `--soloUMIdedup 1MM_CR`,
so there is no combination this bypasses.
`cellranger_1mm_map` exposes the correction mapping that
`cellranger_1mm` already computed and threw away.
Measured against **CellRanger 10.0.0** on the 20 000-read fixture from
#172, with #165 and #173 also applied:
identical entries CellRanger rustar
#165 + #173 13 651 / 13 709 15 111 15 091
plus this change 13 676 / 13 709 15 111 15 116
Entries CellRanger has and we do not go from 29 to 7, and the count gap
from -20 to +5, which is 0.03%.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(divergence): drop a reference to a test that no longer exists
* feat(solo): bit-exact libc++ mt19937, generate_canonical and discrete_distribution
STARsolo's `EmptyDrops_CR` rescue draws from `std::mt19937`, converts to
doubles with `std::generate_canonical<double, 53>`, and picks categories with
`std::discrete_distribution`. Two of those three are implementation-defined in
the parts that matter: the standard fixes mt19937's output but not how
`generate_canonical` consumes it, and says nothing about how
`discrete_distribution` maps a uniform onto categories.
So porting "the algorithm" is not enough — it has to be libc++'s algorithm,
because that is what STAR is built against and where its numbers come from.
libc++ accumulates two 32-bit draws in *ascending* significance and divides by
2^64; a most-significant-first accumulation, or one draw scaled to 53 bits,
both give perfectly good uniforms and neither reproduces STAR.
Every expected value in the tests came out of a C++ program compiled against
the real libc++ and run, not from reading its source. `tests/libcxx_oracle.cpp`
is that program, kept so the values can be regenerated rather than trusted.
`generate_canonical` is compared as bit patterns, since a difference in the
last place changes which category a sample lands in.
Not yet wired into the EmptyDrops path. `solo::count` samples with a
`SplitMix64` stream under a comment calling it "WeightedIndex-equivalent;
empirically byte-identical EmptyDrops cell calls" — a claim that cannot hold in
general, since two unrelated generators cannot agree on an arbitrary number of
draws. It is true of whatever was checked and unknown elsewhere. Replacing it
moves cell calls, so it belongs in its own change with the solo differential
run against it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* feat(solo): EmptyDrops_CR uses Simple Good-Turing and libc++'s sampler
Two approximations in the CellRanger cell-calling path are replaced by what
CellRanger and STAR actually compute. Both move cell calls, which is the point:
the previous numbers were plausible rather than right.
The ambient profile is now smoothed with Simple Good-Turing (Gadsby & Sampson,
via Elworthy's implementation, which is what STAR vendors). The ambient counts
come from a small sample of empty droplets, so a gene seen twice there is not
twice as likely as one seen once, and a gene seen zero times is not impossible —
it is one the sample was too small to show. SGT fits the frequency spectrum and
reserves mass for the unseen from the singleton rate, then smooths the rest
along a log-log line. What was here before had the right shape and the wrong
numbers: it reserved mass the same way but distributed the remainder in
proportion to raw counts, with no smoothing at all.
The Monte-Carlo null is now drawn with libc++'s `std::mt19937` and
`std::discrete_distribution`, seeded `19760110 * (isim + 1)` per simulation, as
STAR seeds it. The previous sampler was a SplitMix64 stream under a comment
calling it "WeightedIndex-equivalent; empirically byte-identical EmptyDrops cell
calls" — a claim that cannot hold in general, since two unrelated generators
cannot agree over an arbitrary number of draws. The libc++ types were ported and
checked against real libc++ in the previous commit on this branch; this wires
them in. One generator per simulation, no shared state, so the walks still run
in any order on any number of threads and give the same p-values.
D17 comes with it: STAR leaves `PZero` uninitialised when the spectrum has fewer
than five distinct frequencies and `analyse()` bails, so it reads whatever the
stack held. Here it is zero from construction, which is what "no basis for
reserving unseen mass" means. Recorded in docs-old/dev/divergences.md.
* docs: record the EmptyDrops SGT divergence in DIVERGENCE.md
Section 1.2, in the What STAR does / What rustar-aligner does / Why / Impact /
Source format CONTRIBUTING.md asks for, replacing the docs-old file the earlier
version of this work carried.
* docs(divergence): file the EmptyDrops entry under section 1, note the
second RNG
* fix(params): refuse MultiGeneUMI_CR without --soloUMIdedup 1MM_CR
* feat(solo): --soloFeatures Transcript3p, with --soloClusterCBfile
Quantifies transcripts rather than genes, from where each read's 3' end sits
relative to each transcript's. In a 3'-biased assay that distance is what
separates isoforms: a read 200 bases from the end of one and 4000 from the end
of another is evidence for the first. The distribution of those distances is
estimated from the run's own histogram, smoothed and cut where the 3' peak
decays into the body, and used as the likelihood in an EM over UMIs.
Concordance needed no new code. `align_to_transcripts` already refuses to
project an alignment that leaves the transcript, touches an intron, or crosses a
junction the transcript does not have — which is exactly STAR's `Concordant`
(`Transcriptome_classifyAlign.cpp`). A projection that survives is concordant;
one that does not, is not. The projection also puts the 5' end at coordinate
zero for both strands, so the distance to the 3' end is one expression rather
than two.
Two behaviours worth stating because they are not the obvious ones:
Output is per cluster, not per cell, and `--soloClusterCBfile` is required.
A single cell does not have enough UMIs to resolve isoforms, so the EM would be
fitting noise. Asking for the feature without a clustering is refused rather
than run.
A UMI seen on several reads contributes the *intersection* of their transcript
sets. Those reads came from one molecule, so a transcript missing from any of
them cannot be its source. Taking the union would let a single stray read
resurrect an isoform every other read excluded.
Two of STAR's quirks are reproduced rather than corrected, because the cut point
and every weight depend on them: the running-average divisor is `min(2N+1,
i + N)` rather than the number of elements actually summed, and the transcript
length factor is taken from the cumulative distribution at `trLen - 1`
(`SoloFeature_quantTranscript.cpp`).
Numbers are formatted the way C++'s default stream prints them — six
significant digits, fixed inside `[1e-4, 1e6)` and scientific outside — since
the normalised distribution runs down to ~1e-4 where Rust's `{}` and C++'s
default disagree on both notation and digit count.
* refactor(solo): drop Transcript3pAcc::merge, which nothing calls
Records are accumulated under a mutex, so there are no partials to merge. It was
dead from the moment it was written; CONTRIBUTING.md rules out shipping it.
* docs(solo): note that STAR marks Transcript3p under development
parametersDefault puts both Transcript3p and --soloClusterCBfile between
"#####UnderDevelopment_begin : not supported - do not use" and
"#####UnderDevelopment_end", and STAR --help prints that banner around
them. The module said none of this.
It matters for how the port is read: it follows STAR's code, so it
inherits the unfinished parts of that code, and a differential against
STAR compares two implementations of something STAR does not support.
A reviewer should be told that before deciding to take it.
* fix(solo): implement MultiGeneUMI_All instead of aliasing it to MultiGeneUMI
`--soloUMIfiltering MultiGeneUMI_All` resolved to the same variant as
`MultiGeneUMI`, which is neither what STAR does nor what the option is
documented to do. Of the three available behaviours it was the only one nobody
had asked for.
In STAR the option is a no-op: it is parsed and stored, but its consumption site
tests only the `MultiGeneUMI` flag, so selecting it leaves the filter entirely
off. Documented, it removes a UMI seen in more than one gene from *all* of them,
rather than from the losers only.
`UmiFiltering::MultiGeneUmiAll` now exists and does the documented thing: a UMI
appearing in several genes is evidence of a collision or of chimeric
amplification, so it is discarded outright rather than attributed to whichever
gene happened to read deepest. Single-gene UMIs are untouched, which the test
checks across every mode.
Raised upstream as #144 before changing it, since "be faithful to STAR" and "do
what the flag says" genuinely point in opposite directions here.
Also adds `docs-old/dev/divergences.md`, recording this and the homopolymer-UMI
rule, so deliberate differences are written down rather than rediscovered as
surprises in a differential run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(changelog): record the MultiGeneUMI_All fix
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(divergence): correct the MultiGeneUMI_All entry, defer the
homopolymer one
---------
Co-authored-by: Benjamin Demaille <benjamin.demaille@icloud.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
STARsolo barcode resolution: the
cbMinPposterior threshold, theoneExactguard,--soloCBtype, adapter-anchoredCB_UMI_Complexgeometry, and--soloOutFormatFeaturesGeneField3.Replaces #150, rebased onto current
main. Opening a new PR rather than force-pushing that branch, so the review history there stays readable. Every objection @Psy-Fer raised on #150 is addressed below, including one that was a real correctness bug.What #150 got wrong, and what changed
The adapter search took the leftmost acceptable match; STAR takes the best-scoring one. @Psy-Fer's diagnosis was exact, including why the tests missed it.
localAlignHammingDist(SequenceFuns.cpp:341) scores every offset and keeps the fewest-mismatch one:The old code returned the first offset inside the mismatch budget, which is the opposite rule: an early approximate match beat a later exact one. This is not cosmetic.
CB_UMI_Complexmeasures its barcode positions from the adapter, so the wrong offset shifts the whole barcode and changes what the read is counted as.Reading that function properly surfaced two more details the old code also had wrong:
Nin the adapter is never a mismatch. It is a wildcard in the query, not a base that fails to match, and charging it spends budget STAR does not spend.STAR also seeds
distBestwith the query length, so an offset mismatching everywhere can never win. That only shows when the budget is as large as the adapter, where it keeps the answerNoneinstead of an arbitrary offset. Reproduced.find_adapter_takes_the_best_scoring_match_not_the_leftmostis the discriminating case the old tests lacked: a one-mismatch match at offset 0 against an exact one at offset 5. Leftmost gives 0, STAR gives 5. Plusfind_adapter_breaks_ties_leftmostandan_n_in_the_adapter_is_not_a_mismatch.--soloChemistryand--soloCBwhitelistDirare gone. Both objections were right and they were the same mistake: neither flag exists in STAR. They came from the STAR-rs donor, which invented them, and #150 presented them as parity work without saying so. PerCONTRIBUTING.mda non-STAR flag needs explicit sign-off and must never be presented as faithfulness, so rather than relabel them I reverted them. If chemistry presets are wanted they should be their own proposal, argued as an addition.DIVERGENCE.mdhad no entry, and needed one. Added §1.2, with the STAR source checked rather than recalled: rustar rejects all four homopolymer UMIs, STAR underCB_UMI_Complexrejects only poly-A. The reason is initialisation order, not a threshold. The four packed constants are built in theSoloReadBarcodeconstructor (SoloReadBarcode.cpp:16-21) frompSolo.umiL, but that path does not learn its UMI length until it has extracted one (SoloReadBarcode_getCBandUMI.cpp:353-354), so all four constants stay0. Poly-A packs to0and is still caught; poly-C/G/T pack non-zero and match nothing (:139). This is not new behaviour in this PR, rustar always rejected all four; what is new is that it is deliberate, locked byumi_valid_rejects_every_homopolymer, and written down.The rest of the diff
cbMinP = 0.975and theoneExactguard. STAR accepts a multi-match CB correction only when the posterior clears 0.975 (SoloReadBarcode_getCBandUMI.cpp,CB_MIN_P), and only when no exact whitelist match was available. The posterior itself already existed inresolve_multi_cb; the threshold and the guard did not, so a low-confidence correction was accepted where STAR drops the read.--soloCBtype. AcceptsSequence(default) andString.Adapter-anchored
CB_UMI_Complexgeometry, positions measured from the adapter match rather than from the read start, which is what--soloAdapterSequenceand--soloAdapterMismatchesNmaxare for.--soloOutFormatFeaturesGeneField3, the third column offeatures.tsv, with-suppressing it.One thing to flag rather than hide
CONTRIBUTING.mdsays one theme per PR. Five of the six commits are barcode and UMI resolution;--soloOutFormatFeaturesGeneField3isfeatures.tsvoutput, written by the same code path but arguably a different area. I have kept it here rather than split it because it is 51 lines and touches no shared logic, but say the word and I will lift it into its own PR.Verification
Unit tests in
src/solo/mod.rs:find_adapter_takes_the_best_scoring_match_not_the_leftmostfind_adapter_breaks_ties_leftmostan_n_in_the_adapter_is_not_a_mismatchIn
src/solo/whitelist.rs:umi_valid_rejects_every_homopolymer— all four bases, plus a UMI that merely starts with a run, plusNstill failing on its own groundsIn
src/solo/count.rs:cb_posterior_below_min_p_is_rejecteda_cb_never_seen_exactly_is_refused_without_pseudocountsGate on this branch, at the commit pushed: 570 lib + 26 integration tests,
cargo clippy --all-targets -- -D warnings,cargo fmt --check, MSRV 1.89 — all green.No new dependency.