integrate hyalite v0.2.0 for CellRanger4 - #198
Conversation
|
It would be good to get your feedback on this one @BenjaminDEMAILLE Cheers, |
BenjaminDEMAILLE
left a comment
There was a problem hiding this comment.
Reviewed the full diff and checked both halves against STAR's own sources (ClipCR4.cpp, ClipMate_clipChunk.cpp, ClipMate_clip.cpp, ParametersClip_initialize.cpp). This is a solid piece of work: the oracle-driven approach is the right way to land a clip rule, and catching that cr4_tso_matches_star_opal_oracle had been passing vacuously is a good save on its own.
What I verified independently
poly_tail_3pis an exact transcription ofClipCR4.cpp:82-107, including theib1 = seqLen-1initialisation and the unsigned semantics ofib - score > 27. Correct.- Scoring matrix,
gapOpen = gapExt = 2,alphabetLength = 5,readLen = 91, and the N-padding all matchClipCR4()andopalFillOneSeqexactly. - The 5' gate matches
ClipMate_clipChunk.cpp:44-47, andmin(clip, Lread)matchesClipMate_clip.cpp:51. - Determinism holds:
par_chunks+flat_map_iter+collectis order-preserving, the chunking is independent of--runThreadN, and there is no cross-read dependence (each target is padded to 91 in isolation), so batch composition cannot move a result. Database::scan_alldoesout.clear(); out.reserve(sequences.len())and pushes one hit per sequence, so thechunk.iter().zip(&hits)is safe by construction rather than by luck.
Things I would like changed before merge
1. see TODO.md points at a gitignored file. src/solo/mod.rs:608 refers readers to TODO.md, which .gitignore:40 excludes. No contributor can follow that reference. Please point at the tracking issue number instead.
2. DIVERGENCE.md is not updated. CONTRIBUTING.md:54 requires an entry for any deliberate divergence, and there are two live here: the CellRanger4 + clip5pNbases position bug, and CellRanger4 being solo-only (see 3).
3. CellRanger4 is a silent no-op outside --soloType. STAR arms clip types 10/11 in ParametersClip_initialize.cpp:16-31 unconditionally and applies them in ClipMate::clip for every run, solo or not. rustar only branches on cr4_clip inside align_reads_solo (src/lib.rs:2029), so a plain alignReads --clipAdapterType CellRanger4 clips nothing and says nothing. That is pre-existing, but this PR is where it becomes a deliberate choice (#148's clip_mate wiring dropped, and #148's warning removed with it). CONTRIBUTING.md:64 is explicit that a flag which parses but does nothing must not be quietly accepted. A warning at parse time when CellRanger4 is set without --soloType would close it.
4. The NB comment about the 5' window is not right, and I think it points away from the real bug. Working from the sources: STAR does the fixed N clip first and then clippedAdN = min(clippedInfo, Lread) (ClipMate_clip.cpp:14-24 then 50-54), while rustar does the TSO clip first and the fixed clip second. Both remove N + L bases from the front, and the min only binds once the read is exhausted, in which case both paths land on an empty read. So for the 5' end the two orders are equivalent, and only the 3' window genuinely differs (STAR runs polyTail3p after clip3pNbases).
That is consistent with what you observed: narrowing the window made agreement worse because the window was never the cause. My suggestion for the follow-up investigation is the soft-clip bookkeeping instead, specifically total_clip5p = cr4_5p + clip5p where clip_read has already shifted the sequence, since a double count there would produce exactly a ±clip5pNbases POS delta while leaving mapped counts almost unchanged.
5. hyalite = "0.2" as a caret range. rustar-aligner is published to crates.io, so downstream consumers resolve their own 0.2.x without this repository's Cargo.lock. On a crate that is a week old with a single maintainer, an unvetted patch release can move alignment output. I would pin =0.2.0 (or ~0.2.0) until it has some track record. More on the dependency itself in #197.
Nits
synthetic_cr4_reads()returns 500 reads, just underCR4_SCAN_CHUNK = 512, so the unit-level batch tests never cross a chunk boundary. The 938-read oracle does cross it, so the coverage exists, but only incidentally. Bumping the synthetic corpus above 512 would make it deliberate.- A
debug_assert_eq!(hits.len(), chunk.len())intso_clip_lens_cr4_batchwould document thescan_allcontract theziprelies on. - The PR is currently in a conflicting state and needs a rebase (CHANGELOG).
--clipAdapterType CellRanger4now matches STAR exactly, both halves of the clip.Closes #148. Dependency discussed in #197.
What changed
5' TSO trim (new)
Previously a heuristic: clip the full 30 nt TSO if the read prefix matched it within 3 mismatches. STAR instead runs an overlap alignment of the TSO against the first 91 bases of the read (
ClipCR4::opalAlign, via Opal) and applies an acceptance gate.solo::tso_clip_len_cr4now replicates that setup exactly — ACGTN alphabet; match +1, mismatch −2, any-vs-N −2, N-vs-N 0; gap open = gap extend = 2; overlap mode with end tracking; target N-padded to 91 (ClipCR4::opalFillOneSeq) — then STAR's gateS<20 || (S==20 && L>26) || (S==21 && L>30)(ClipMate_clipChunk.cpp:44-47) andmin(clip, Lread)(ClipMate_clip.cpp:53).3' poly-A trim (corrected)
Was "trim a trailing run of A ≥ 8". STAR's
ClipCR4::polyTail3pis a scored, mismatch-tolerant walk: +1 per A, −2 per non-A, accept whilescore*10 >= ib*7, give up atib-score > 27, reject unless the kept score reached 20, and never trim a read under 20 bases.The old rule was wrong in both directions:
A*15 + C + A*15The Rust transcription is Benjamin Demaille's from #148 and is correct — I re-derived all four of its hand-written expectations against STAR's compiled
polyTail3p, including the non-obvious34forACGT*5 + A*30, and they all hold. It is credited in the doc comment.I did not merge #148 itself, because it wires the trim into
clip::clip_mate, which the solo path never calls (solo goes throughparams.clip5p(0)+clip_read+solo::clip_adapter_cr4) — it would have been dead code on the only path whereCellRanger4is reachable. #148 also logs a warning that the TSO trim is unimplemented, and its CHANGELOG saysCellRanger4is "rejected at parse time"; neither is true (params/mod.rs:1716accepts it, and #148's own code applies the trim). Nothing in #148 is left unlanded.New dependency: hyalite 0.2 (#197)
The overlap alignment comes from
hyalite0.2 — pure Rust, zero dependencies of its own, so it adds one node to the tree and no C toolchain requirement.Reads are scanned a batch at a time through
Database::scan_all, mirroring STAR, which fills a 64-read Opal "database" per chunk and aligns the adapter against all of them in one call (ClipMate::clipChunk).solo::tso_clip_lens_cr4_batchdoes the same over rayon chunks;align_reads_soloresolves the whole batch up front and indexes the result per read.This is a change to a hot path, so: measured over 100k reads including the per-chunk
Databasebuild, batchedscan_allis 7.5× faster on AVX2 (4.7× SSE4.1) than per-readalign_pair, bit-identical. Throughput is flat from chunk 128 to 10k, so the chunk size is picked to give rayon enough work units, not to tune SIMD.Validation
Both halves are gated against STAR's own C++, not hand-written expectations.
tests/data/cr4_opal_oracle.cpplinks STAR'sopal.cppand reproducesClipCR4+ClipMate::clipChunkverbatim; the committedcr4_opal_oracle.tsvis its output over 938 reads straddling both decision boundaries — the 5' gate (TSO with 0–14 mismatches, shifts, truncations, indels, embedded Ns, short reads) andpolyTail3p's own (score-20 floor, 70% density,ib-score>27give-up,seqLen<20, A-rich sequence upstream of the tail). The regeneration recipe is in the.cppheader; CI needs no Opal build.cr4_tso_matches_star_opal_oraclechecks all 938 against the scalar path, a forced-SIMD path, and the production batch path. It previously readRUSTAR_CR4_ORACLEand silently returned when unset, so it had been passing vacuously; it now defaults to the committed fixture, with the env var as an override.Differential vs STARsolo — 10x mouse chr19, both tools with
--clipAdapterType CellRanger4:5' leading-clip diffs are 0. The 15 remaining 3'-tail diffs are all STAR extending a short terminal segment across a junction or indel (
9S→241N9M,12S→2I10M) — annotated-junction extension, a separate known class, not a trim disagreement. Solo Gene matrix: 383/384 shared entries, 382 with identical counts, 442 vs 441 total UMIs.Default SE/PE unchanged —
CellRanger4is opt-in and nothing else moved:Both exactly the recorded baselines.
Tests
New/changed, all present in the diff and passing:
cr4_tso_matches_star_opal_oracle— the 938-read STAR differential, both columnscr4_tso_scan_all_batch_matches_scalar_and_is_backend_stable— batch vs scalar, every available backend, plus the production entry pointcr4_tso_database_is_deterministic_across_backends_and_matches_scalarcr4_polya_trim_follows_star_not_a_run_length_rule— the poly-A cases above, and that the scan runs on the post-5'-clip readcr4_tso_clip_removes_exact_tso_prefix,cr4_tso_clip_is_noop_without_adapter,cr4_tso_clip_handles_short_and_empty_readstest_starsolo_cr4_tso_clip_is_per_read_within_batch— end-to-end, interleaving TSO-bearing and TSO-free reads so a misindexed batch scan lands a clip on the wrong read594 tests pass, 0 clippy warnings,
cargo fmt --checkclean.Known limitation (pre-existing, not from this PR)
CellRanger4combined with a non-zero--clip5pNbases/--clip3pNbasesmisplaces most reads byclip5pNbases: on the same dataset the rustar-minus-STAR POS delta is −5 on 8527 reads, +5 on 3546, and 0 on only 552. It is specific to the pairing — the fixed clips alone are fine (1 leading-clip diff, ~980 other, i.e. the usual baseline), andCellRanger4alone is fine (0 clip diffs, above). Mapped counts stay close (14747 vs 14687), so reads are misplaced, not lost.This predates the PR and is not made worse by it. It is filed separately, and it matters for #176, which would make
CellRanger4a default on 10x geometry — at which point users would hit this without ever opting into CR4. It should be fixed before #176 merges. Still not sure what the cause is, so will need to investigate.One dead end worth recording: STAR does run
polyTail3pafter the fixed trims, so our scan window is formally wrong for that pairing. I tried narrowing it to match and agreement got worse (475 → 549 leading-clip diffs), because the position bug swamps it. Reverted; there is an NB comment in the code.