Skip to content

Ben consolidation 2 - #135

Merged
Psy-Fer merged 4 commits into
mainfrom
ben-consolidation-2
Jul 26, 2026
Merged

Ben consolidation 2#135
Psy-Fer merged 4 commits into
mainfrom
ben-consolidation-2

Conversation

@Psy-Fer

@Psy-Fer Psy-Fer commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

Batch 2: adapter clipping + STAR-faithful soft-clips + coverage signal (closes #113, #114)

Second consolidation batch of BenjaminDEMAILLE's STAR-rs-ported PRs. Each PR's logic was reviewed against real STAR (not trusted on STAR-rs provenance), the flagged bugs were fixed, and every change is byte-validated against STAR on the yeast test set.

Closes #113, #114

#113 — adapter-aware read clipping (--clip3pAdapterSeq)

Ported from STAR-rs, but the review found the headline feature was dead code: reads reaching clip_mate are already numeric base codes, but it called encode_base on them again → every base became N → the 3' adapter Hamming scan never matched → nothing was ever clipped. The unit tests passed only because they fed ASCII string literals, a path production never hits.

  • Fix: encode the adapter once in clip_params_from (it's the ASCII CLI value); clip_mate now compares the already-numeric read against the numeric adapter with no re-encoding.
  • Wired into the current pipelined loop (the PR was written against the old sequential loop): SE + PE non-solo, per-mate (clip_params_from(params, mate) — the fixed --clip{5,3}pNbases are per-mate).
  • Added a regression test that feeds numeric reads like the real pipeline.

Soft-clip convention — align rustar's clip output with STAR

Investigating #113 surfaced a broader, pre-existing divergence: rustar dropped clipped bases from the record, while STAR (and STAR-rs) soft-clip them — the mapped read is the clipped insert, but the output SEQ keeps the full read with leading/trailing S in the CIGAR. (Notably: on clipping, STAR-rs is byte-identical to STAR — rustar was the one diverging. The drop was an early phase-6 shortcut, no real reason.)

This is now fixed to match STAR:

  • New apply_read_clips in io/sam.rs: the record is built from the clipped read (so CIGAR core / MD / NM stay correct over the aligned bases), then the full original read is restored as SEQ/QUAL and the CIGAR is wrapped with strand-aware soft-clips (a read-3' clip becomes a leading S on a reverse-strand record; adjacent soft-clips are merged).
  • Applies to mapped SE + PE reads. Unmapped reads (SE + PE) now emit the full original read too (STAR convention — clipping never removes bases from an unmapped record).
  • Solo clip paths intentionally left on the drop path (pass 0,0) to preserve the batch-1 byte-exact STARsolo validation; inert on default 10x (no cDNA clipping). Tracked in TODO.
  • Half-mapped mapped-mate soft-clip is the one remaining edge case (rare: clipped mate maps, partner doesn't) — left valid-but-dropped, tracked in TODO.

#114 — coverage signal (--outWigType bedGraph)

Ported from STAR-rs's star_wig.rs (STAR's signalFromBAM.cpp): four stranded Signal.{Unique,UniqueMultiple}.str{1,2}.out.bg tracks, RPM (default) or raw.

  • Integrated into the pipelined writer thread — the PR targeted the old sequential loop; the coverage accumulator + RPM counters now live in the writer thread (they aren't safe to update from the parallel per-read closures).
  • PE RPM 2× bug fixed (the review's main finding). STAR's signalFromBAM counts BAM records, so both mates of a pair contribute to nUniq/nMult; the PR counted once per pair, halving the denominator and doubling every PE RPM track. Fix: per mate-record — a uniquely-mapped pair adds 2 to nUniq, a multimapped pair adds 2 to nMult. SE stays +1 (a multimapper's records sum to exactly 1: NH × 1/NH), verified against signalFromBAM.cpp:22-28.

Validation

Byte-compared against STAR 2.7.11b (and STAR-rs) on synthetic + 10k yeast reads:

Check Result
Adapter clip, SE forward 137M13S, full SEQ — byte-identical to STAR
Adapter clip, SE reverse 13S137M (leading soft-clip) — byte-identical
Adapter clip, PE (both mates) 99/137M13S + 147/13S137Mbyte-identical
--outWigType bedGraph PE RPM Signal.*.str1 byte-identical; total ratio 1.0000 (was 2.0)
--outWigType bedGraph SE byte-identical
No regression (default SE / PE) SE 8788, PE 16582 — unchanged, byte-identical to baseline

(The str2 bedGraph differs by ~0.002% — one read's coverage — from the known PE alignment tie-break divergence bleeding into coverage, not a signal bug.)

Gate: 559 tests pass · clippy --all-targets 0 warnings · fmt clean.

BenjaminDEMAILLE and others added 3 commits July 25, 2026 17:56
Port STAR-rs's Hamming-mode adapter clipping from STAR-rs
`crates/star-align/src/star_clip.rs`, extending rustar-aligner's existing
fixed-count `--clip5pNbases`/`--clip3pNbases` with a 3' adapter scan and the
after-adapter trim.

- src/clip/mod.rs: ClipEnd/ClipParams + clip_mate (STAR's two ClipMate passes:
  5' then 3' on the 5'-clipped read) + local_search (STAR's localSearch Hamming
  scan). clip_params_from builds the config once per run from Parameters.
  Scoped to --clipAdapterType Hamming (STAR's default and the only mode that
  supports a 3' adapter); CellRanger4 (10x TSO + poly-A trim) is deferred to
  its own PR alongside STARsolo, which rustar-aligner doesn't have yet.
- src/params/mod.rs: --clip3pAdapterSeq (`-` = none, matching STAR's own CLI
  sentinel), --clip3pAdapterMMp, --clip3pAfterAdapterNbases,
  --clip5pAfterAdapterNbases.
- src/lib.rs: build ClipParams once per run in both the single-end and
  paired-end loops; clip_mate now runs per read (each mate's adapter position
  is scanned independently) in place of the old fixed clip5p/clip3p constants.
  Output behavior otherwise unchanged: the clipped read is fed to alignment
  and SAM output as before, clipped bases are not soft-clip-reinserted
  (matches the pre-existing --clip5pNbases/--clip3pNbases convention).

Also fixes CLAUDE.md's source layout, which omitted src/wasp/ (missed when
condensing the doc in 8c28a4c).

Gate green: cargo build, clippy -D warnings, fmt, 470 tests (6 new clip unit
tests). Byte-level validation against native STAR via the test/ harness is a
separate step.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Psy-Fer Psy-Fer self-assigned this Jul 26, 2026
@Psy-Fer
Psy-Fer enabled auto-merge (squash) July 26, 2026 08:55
@Psy-Fer
Psy-Fer merged commit f22d151 into main Jul 26, 2026
10 checks passed
@Psy-Fer
Psy-Fer deleted the ben-consolidation-2 branch July 26, 2026 08:59
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