fix: count secondary+supplementary records as secondary only (flagstat) - #131
Open
BenjaminDEMAILLE wants to merge 1 commit into
Open
Conversation
samtools gives the SECONDARY (0x100) flag priority over SUPPLEMENTARY (0x800): a record carrying both is counted as secondary and never reaches the supplementary counter (bam_stat.c flagstat_loop and stats.c both return early on secondary). RustQC tested the two bits independently, so dual-flagged records were counted twice — inflating the flagstat supplementary total and breaking the invariant `primary + secondary + supplementary == total`. The same conflation affected the samtools stats SN section: "non-primary alignments" is `nreads_secondary` in samtools, not secondary+supplementary. Verified against samtools 1.24 on a synthetic BAM with 4 primary pairs, 2 secondary-only, 3 supplementary-only and 5 dual-flagged records: 18 total / 8 primary / 7 secondary / 3 supplementary, non-primary 7. Closes seqeralabs#125 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Fixes #125.
Problem
Records carrying both
SECONDARY(0x100) andSUPPLEMENTARY(0x800) were counted in both thesecondaryandsupplementaryflagstat totals, because the two bits were tested independently:samtools gives
SECONDARYpriority —bam_stat.c(flagstat) andstats.cboth classify such a record as secondary and never reach the supplementary counter. The double count inflates the supplementary total and breaks the invariantprimary + secondary + supplementary == total.While checking this against samtools, a second instance of the same conflation showed up in the samtools stats
SNsection:non-primary alignmentsisnreads_secondaryin samtools, but RustQC emittedsecondary + supplementary. That line was wrong for any BAM containing supplementary alignments, dual-flagged or not.Fix
accumulators.rs:if is_secondary { .. } else if is_supplementary { .. }stats.rs:non-primary alignments:now reportssecondaryaloneVerification
Ground truth from
samtools 1.24on a synthetic BAM (4 primary pairs, 2 secondary-only, 3 supplementary-only, 5 dual-flagged):A second control file with the dual-flagged records removed (2 secondary-only, 3 supplementary-only) gives
non-primary alignments: 2, confirming that line tracks secondary alone rather than the sum.New unit test
test_dual_flagged_reads_count_as_secondary_onlybuilds the same record mix and asserts those numbers plus the sum invariant.cargo test— 201 lib + 18 integration tests passcargo fmt --check,cargo clippy -- -D warningsclean🤖 Generated with Claude Code