feat(quant): fill in the mapping-output fields SAM allows to be absent (QUAL, M5/UR, MC/MQ, ZW, @RG, unaligned reads) - #1142
Open
BenjaminDEMAILLE wants to merge 5 commits into
Conversation
rob-p
marked this pull request as draft
August 22, 2026 20:03
This was referenced Aug 22, 2026
rob-p
pushed a commit
that referenced
this pull request
Aug 24, 2026
Follow-up to the #1151 review, which landed before the change could go in. The `nh == 0` arm returned 255. No mapped record reaches it (a fragment with no placements never enters the record loop), so this is about the record kinds that have no placement by construction: an unmapped read, once such records are written (#1142). MAPQ 255 on an unmapped record claims certainty about a placement that does not exist, and 0 is what samtools convention expects there. Whoever adds unmapped output should not have to remember this function's edge case, which is the argument for fixing it now rather than then. Refs #1151. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 24, 2026
Mapping runs ksw2 in KSW_EZ_SCORE_ONLY: it needs the score to rank and filter placements, not the path that produced it, and skipping the traceback matrix is the right trade for a kernel that runs once per candidate across the library. That is also why no CIGAR survives to the record writer, and why the writer has been synthesizing one from the read length. This module re-aligns a placement with traceback, on demand, producing the CIGAR plus the edit distance and MD string that describe it. Nothing calls it yet. Three things keep it affordable, since it will run once per written record: * Provably-ungapped fast path. Any alignment containing a gap pays at least gap_open + gap_extend and can do no better than match everywhere else, so it cannot exceed perfect - (gap_open + gap_extend). The ungapped alignment scores perfect - mismatches * (match + mismatch). When the second is at least the first, no gapped alignment can win and no DP runs at all. At salmon's defaults that admits up to one mismatch, which covers most reads at realistic error rates. * Traceback skipped when the banded optimum equals the ungapped score over the full read: filling and walking the traceback matrix is the expensive half. * Anchor repair by comparison rather than by DP. The mapper's position comes from a seed chain, which a mismatch near the 5' end pushes rightwards; sliding the read and counting mismatches finds the true start directly, unbounded by any band. An exact probe locates candidates first, with the exhaustive sweep as the fallback for the fraction of a percent the probe cannot place. The `ambiguous` slice names reference offsets whose base indexing had to replace, so NM and MD can report the base the user supplied rather than the substitute. Callers pass an empty slice until the index records them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Indexing replaces non-ACGT bases with ACGT, as salmon has always done, because the k-mer structure cannot hold them. Nothing recorded where, so an alignment over such a position counted a match where the input had an N, and MD named the substitute instead of the base the user supplied. The build now writes ambiguous.bin, the offsets it replaced: 40 bytes for the whole human transcriptome. Alignment still runs against the cleaned sequence, the only thing it can do, but a record can now report what was actually there. An index built before this file existed loads as "none recorded" and behaves exactly as it did, so nothing has to be rebuilt to keep working; rebuilding is what earns the correction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
--writeMappings has always been a diagnostic view of salmon's mappings rather than a general-purpose aligner output, and that is the right scope. But the view stated things that were not true of the mappings it described. * The CIGAR was synthesized from the read length: <readLen>M plus a clip at a transcript end. A placement with an indel was written as if it had none, so every base past the gap sat at the wrong reference offset, and the record contradicted its own placement. * AS was the mapper's chain score, and nothing else in the record could be checked against it. Auditing AS against a recomputed NM over a whole output found 2.4% of records self-contradicting: a 15M1I1D15M1I1D... sawtooth at NM:i:80 on a 100 base read, beside an AS claiming about three mismatches. * MAPQ was the constant 1, so `samtools view -q 255`, the standard "uniquely placed" filter, discarded the entire file. Records now carry the CIGAR of an alignment actually realized for that placement, with real I/D operations, the NM and MD that describe it, the score of that same alignment, and MAPQ derived from the placement count on STAR's scale (255 unique, 3, 1, 0), which is what the tools downstream already filter on. Three separate causes behind the self-contradicting records, each of which alone produces a false one: * The anchor search reached only indel_margin. That bounds indel size and says nothing about how wrong a seed-derived position can be: a chain on a repeat or a paralogous stretch can imply a start most of a read away. One offending read belonged 69 bases earlier, where it matched with three mismatches. Out of reach, the banded DP still returned an alignment: a sawtooth of one-base indels walking the read diagonally across the reference. The search now covers a read length either side. * The search gave up entirely when the read did not fit inside the reference at its anchor, which is exactly the case most worth looking at: an anchor near a transcript end becomes a soft-clipped record that throws bases away when the read often belongs further in and matches in full. "Does not fit" is now treated as "matches nothing" so the search runs. * ksw2's extension pins the read's first base to the window's first base, so a realized alignment can grow rightwards but never shift left, and a mismatch near the 5' end left the DP no way to say "the read started earlier" except by opening a leading insertion: 26I34M at NM 26 where the answer is 60M at NM 1. A leading insertion of n bases is itself the statement that the read began n bases earlier, so it is also the correction. A repaired anchor is a hypothesis, not a verdict: a read with a genuine deletion also looks badly placed to an ungapped scan, so both anchors are realized and the better score wins. An alignment scoring below the mapper's own min_accepted_score for the bases it claims is refused, and the record falls back to the synthesized CIGAR with no NM or MD, which at least says "no base-level alignment here". Cost, measured on 1M fragments at -p 8, best of 3: a run without mapping output is unchanged (0.89s), because nothing is realized unless records are written; --writeBam goes from 1.04s to 1.94s on a single-mapping fixture and 1.36s to 3.28s at 2.5 placements per fragment, the cost being per record. NM and MD match samtools calmd across 2M records, including for the transcript containing an N. QUAL, M5/UR, MC/MQ, ZW, read groups and unmapped records are all still absent. Those are omissions the format allows, unlike a CIGAR that does not describe the alignment beside it, and they are the subject of a separate change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Everything here is an omission rather than a falsehood: SAM permits `QUAL` to be `*`, and `M5`, `UR`, `MC`, `MQ` and `ZW` to be missing. The file was still less useful than it had to be, and salmon already had every value in hand. * QUAL, taken from the FASTQ and reversed alongside SEQ on the reverse strand, so `samtools fastq` can round-trip the library back out of the BAM. It is not behind a flag: whenever the input carries qualities the record does. A quality string whose length disagrees with the read is dropped rather than written, since that mismatch is what makes a record unreadable, and it warns once. * `@SQ M5`, the MD5 of each reference, and `UR`, the index it came from. M5 is what ties a BAM to the exact bases it was made against, and what CRAM needs. Where indexing had to replace a base the checksum hashes the `N` that was there, so the header names the reference the user supplied. The whole transcriptome is hashed in parallel at startup: 0.16s to 0.08s on a 44 MB transcriptome, and it matches `samtools dict`. * `MC` and `MQ`, the mate's CIGAR and mapping quality, which only became meaningful once the CIGAR was real and MAPQ was not a constant. A mate-aware tool no longer has to sort the file to find them. * `ZW`, the placement's equivalence-class weight, which salmon already computed and nothing else in the file reports. * `@RG` and `RG:Z` via `--rgLine`, in the spelling bwa and STAR users already type, parsed before any mapping work so a malformed line fails immediately rather than after the run. Read groups are how a merged BAM keeps track of which sample each record came from. * `@HD VN:1.6 SO:unsorted GO:query` and a `@CO` stating the one ordering guarantee. `SO:unknown` was not false, but it hid a property the output does have: records for one fragment are contiguous, so a reader can pair mates in a single streaming pass instead of sorting first. `--writeQualities` becomes accepted-and-default rather than warned-as-missing, since qualities are now written whenever the input has them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A BAM containing only the reads that mapped is a record of one filtering decision, not of the experiment, and the reads it drops are exactly the ones anybody debugging a low mapping rate wants. The format has a place for them: FLAG 0x4, no reference, no position, sequence and qualities intact. --sampleUnaligned no longer requires --sampleOut (which remains unimplemented). On its own it adds those records to --writeMappings/--writeBam, and warns when there is no mapping output for them to go into. It also covers an orphan's unmapped mate, which is a read that did not align just as much as one whose whole fragment failed. The mapped mate carries MATE_UNMAPPED, so without the mate's record the file described a read it did not contain: samtools fastq could not round-trip it and anything following the flag chased nothing. The mate is written at the mapped mate's position, so a sort keeps the pair together, and the two records point at each other. Without --sampleUnaligned the mapped record keeps RNEXT '*', since naming a mate that was not written is worse than admitting there is none. With this, samtools fastq round-trips a whole real library out of the BAM: all 457366 pairs of DRR028171 returned, not one sequence or quality string altered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
BenjaminDEMAILLE
force-pushed
the
feat/mapping-output-completeness
branch
from
August 30, 2026 11:44
d30bb1b to
f54ac79
Compare
BenjaminDEMAILLE
marked this pull request as ready for review
September 26, 2026 11:16
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.
Second of three, stacked on #1141. That PR fixes the statements the mapping output makes that are false (a synthesized CIGAR,
ASdescribing a different alignment,MAPQpinned to 1). This one fills in the fields SAM allows to be absent, which is a weaker claim and deliberately a separate decision:QUALmay be*, andM5,UR,MC,MQ,ZWmay simply not be there. The file was still less useful than it had to be, and salmon already had every value in hand.GitHub cannot point a base branch at a fork, so the diff shown here includes #1141 until that merges. The two commits on top of it are the content of this PR (~1.0k lines).
What gets written
QUAL*/0xffSEQon the reverse strand. Not behind a flag: whenever the input carries qualities, the record does. A quality string whose length disagrees with the read is dropped rather than written (that mismatch is what makes a record unreadable) and warns once.@SQ M5Nthat was there (usingambiguous.binfrom #1141), so the header names the reference the user supplied. Matchessamtools dict.@SQ URfile://URI, the shape samtools writes.MC/MQMAPQwas not a constant. A mate-aware tool no longer has to sort the file to find them.ZW@RG+RG:Z--rgLine, in the spelling bwa and STAR users already type (tab-separatedKEY:value, escaped\taccepted), parsed before any mapping work so a malformed line fails immediately rather than after the run.@HDVN:1.0 SO:unknownVN:1.6 SO:unsorted GO:query, plus a@COstating the one ordering guarantee.SO:unknownwas not false, but it hid a property the output does have: records for one fragment are contiguous, so a reader can pair mates in one streaming pass instead of sorting first.--sampleUnaligned:FLAG 0x4, no reference, no position, sequence and qualities intact.--writeQualitiesbecomes accepted-and-default rather than warned-as-unimplemented.--sampleOutstays unimplemented and still warns.--sampleUnaligned, and the orphan mateA BAM containing only the reads that mapped is a record of one filtering decision, not of the experiment, and the reads it drops are exactly the ones anybody debugging a low mapping rate wants.
--sampleUnalignedno longer requires--sampleOut; on its own it adds those records to--writeMappings/--writeBam, and warns when there is no mapping output for them to go into.It also covers an orphan's unmapped mate, which is a read that did not align just as much as one whose whole fragment failed. The mapped mate carries
MATE_UNMAPPED, so without the mate's record the file described a read it did not contain. The mate is written at the mapped mate's position so a sort keeps the pair together, and the two records point at each other. Without--sampleUnalignedthe mapped record keepsRNEXT '*', since naming a mate that was not written is worse than admitting there is none.Cost
M5means hashing the whole transcriptome at startup, a few hundred megabytes of MD5. It is one independent hash per reference, so they run in parallel: 0.16s to 0.08s on a 44 MB transcriptome.QUALand the extra tags are per-record work already dominated by realization; the--writeBamfigures in #1141 (1.94s / 3.28s per 1M fragments) are measured with everything in this PR present.MD:Zis the largest single contributor to file-size growth and is redundant with the reference, so making it optional is the obvious knob if that matters; not done here.Verification
Against samtools 1.24, on 20k unique transcripts and 5k genes x 4 isoforms (1M fragments each), human chr22 (hg38) + GENCODE v44, and real reads (DRR028171, HiSeq 2500, 457366 pairs of 151 bases):
samtools fastqround-trips the whole real library back out of the BAM: all 457366 reads returned, not one sequence or quality string altered. This is the strongest check here, since it holdsSEQ/QUALorientation to account on every reverse-complemented record.@SQ M5matchessamtools dict, and matches the input FASTA even for the transcript containing anN.samtools quickcheckpasses; SAM output round-trips throughsamtools view -b.M5,UR,@RG), the tag set, the read group on every record, the orphan mate, and that unaligned records appear only when asked for.Stack
NM/MD,AS,MAPQ,ambiguous.bin).--spliced), parked pending the demonstrated-need discussion there.