Skip to content

feat(dna): Picard CollectWgsMetrics and CollectInsertSizeMetrics - #154

Open
BenjaminDEMAILLE wants to merge 17 commits into
seqeralabs:mainfrom
BenjaminDEMAILLE:feat/dna-picard-core
Open

feat(dna): Picard CollectWgsMetrics and CollectInsertSizeMetrics#154
BenjaminDEMAILLE wants to merge 17 commits into
seqeralabs:mainfrom
BenjaminDEMAILLE:feat/dna-picard-core

Conversation

@BenjaminDEMAILLE

@BenjaminDEMAILLE BenjaminDEMAILLE commented Aug 28, 2026

Copy link
Copy Markdown

Part of #157.

Third of the DNA stack. Stacked on #153, which is itself stacked on #152; review those first.

Adds the two core Picard metrics to rustqc dna, both computed in the same single pass as everything else.

Parity against Picard 3.4.0

Output Result
insert_size_metrics.txt identical, metrics row and all 170 histogram lines
wgs_metrics.txt identical on every column and all 251 histogram lines, except two

The two exceptions are HET_SNP_SENSITIVITY and HET_SNP_Q, which come from Picard's TheoreticalSensitivity, a Monte Carlo simulation over the base quality and depth distributions. Matching it would mean reproducing its random number generator and sampling order bit for bit, which buys nothing for quality control. Both are written as ?, the marker Picard itself uses for a value it cannot compute, and the parity test permits a difference in exactly those two columns and nowhere else. FOLD_80/90/95_BASE_PENALTY are ? on both sides: Picard could not compute them on this data either.

How the semantics were established

Neither tool's rules were written from memory. Each was derived by reproducing Picard's own numbers on the fixture until every figure matched, and the two that mattered most both turned out to be non-obvious.

Insert size inclusion. Paired, not secondary, supplementary, duplicate or unmapped, mate mapped, positive TLEN so each pair counts once. Proper-pair is deliberately not required: requiring it drops one pair and shortens the maximum from 300 to 239 on this data. MEAN and STANDARD_DEVIATION are over the histogram trimmed to DEVIATIONS median absolute deviations either side of the median with the n-1 denominator, while MIN and MAX are over the untrimmed set. WIDTH_OF_XX_PERCENT grows a window symmetrically around the median until it covers the percentile and reports 2i+1; all eleven match.

The WGS exclusion model. Unmapped, secondary and supplementary records never enter the calculation at all. Every other record's reference-consuming bases form the denominator of all the PCT_EXC_* columns, 670989 bases on the fixture. Exclusions then apply in a fixed order, each counted against that same denominator: duplicate, low mapping quality and unpaired remove a whole read; low base quality and mate overlap remove single bases; depth beyond COVERAGE_CAP is counted as excess. What survives is the high quality coverage the histogram reports, and it reconciles exactly: 670989 − 201120 − 4933 − 217866 − 105814 = 141256.

SD_COVERAGE is the sample standard deviation over every base of the territory, uncovered ones included, not over the covered ones only.

A design point that held up

CollectWgsMetrics gets its own depth accumulator rather than a correction applied to the mosdepth one, because the two tools disagree on both which reads and which bases count. Keeping them separate was the design's stated reason for accepting two accumulators, and reproducing Picard's fractions confirmed it: there is no post-hoc correction that turns one into the other.

Behaviour

CollectWgsMetrics needs --reference to count the reference's non-N bases, which is GENOME_TERRITORY. Without one it is skipped with a warning rather than reported against a wrong denominator; insert size needs no reference and is still written. There is a test for exactly that.

Fixtures are generated with the JVM locale pinned to English. A French default locale writes 3,531312 where an English one writes 3.531312, which would make the fixtures depend on the machine that produced them. Picard's four-line preamble, a command line with absolute paths and a timestamp, is stripped.

Tests

306 green, up from 285. Twenty-four of them are DNA parity tests, at both the library and the binary level.

🤖 Generated with Claude Code

BenjaminDEMAILLE and others added 17 commits August 28, 2026 18:26
These three modules carry no RNA-specific logic and are needed by the
forthcoming dna subcommand. src/rna re-exports them so every existing
crate::rna::... path and the published 0.2.x library surface keep working.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bam_stat is read-level and needs no annotation, and the samtools stats,
flagstat and idxstats writers consume its result type, so all four move
together into src/common/. src/rna/rseqc re-exports them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
BamStatAccum gathers the read-level counters behind bam_stat and the
samtools writers. Its process_read takes only a record and a MAPQ cutoff,
so it is assay-agnostic and the dna pipeline will drive the same struct.
The merge_vec_arrays helper moves with it, being its only consumer.
rna::rseqc::accumulators re-exports the type.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Also corrects the AGENTS.md claim that the crate has no lib.rs, which has
been untrue since seqeralabs#101.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A real public human chr22 slice from nf-core/test-datasets, duplicate-marked
locally with samtools, plus mosdepth 0.3.14 and samtools 1.24 reference
outputs. The generation script pins both tool versions and refuses to run
against others, so fixtures and tool versions cannot drift apart.

380 kB in total, well inside the 10 MB fixture budget.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Shared options keep the same long name, short flag and RUSTQC_* environment
variable as their rna counterparts. The deliberate differences: no --gtf, no
--stranded, and --mapq defaults to 0 rather than 30 because that is
mosdepth's default.

run_dna is a stub for now; the pipeline lands in the following commits.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Config gains a `dna` block alongside `rna`, with mosdepth and samtools
sub-sections and a reuse of the existing PreseqConfig. Shared settings
(chromosome_prefix, chromosome_mapping, sample_name, flat_output) are
declared on DnaConfig itself, mirroring RnaConfig, so the two pipelines
can be configured independently in one file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
DepthAccum records aligned blocks as increments in a delta array the length
of the contig, then a prefix sum turns that into per-base depth in one linear
pass. Filters and CIGAR handling reproduce mosdepth 0.3.14 outside fast mode:
flags 1796 excluded, MAPQ floor applied, M/=/X cover the reference, D/N
advance without covering, and I/S/H/P do not advance.

Mate-overlap correction, the other half of mosdepth's default behaviour,
lands in the next commit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mosdepth counts a base once when both mates of a pair cover it, unless
--fast-mode is given. On the test dataset this is the difference between
469875 and 247878 total covered bases, so it is the dominant behaviour
rather than an edge case.

Pending mates are held in a map keyed by read name, indexed by the position
the outstanding mate was announced at so entries that can never be claimed
are evicted as the coordinate-ordered scan moves past them. A test asserts
the map empties.

Includes an engine-level parity check against the committed mosdepth
fixture: total covered bases and maximum depth both match exactly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six writers plus the per-contig summarisation that feeds them: summary,
global and region distributions, per-base runs, per-window means and
per-window threshold counts. Compressed outputs are bgzf, matching mosdepth.

The distribution emission rule was reverse-engineered from the fixtures and
is the non-obvious part: every depth from 0 up to min(300, max) gets a row
whether or not any base sits at it, above 300 only depths that occur and lie
strictly below the maximum do. So the maximum gets a row when it falls inside
the dense range and none when it does not. The global distribution tops out
at 866 with a maximum of 867, while the region distribution does emit its
maximum of 204.

The region distribution is over windows and their rounded mean depth, not
over bases.

Parity tests drive the library directly and compare every mosdepth output
against the committed fixtures: all eight match, including the 1094-line
global distribution and the 721-interval per-base BED.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One rayon worker per contig, each holding its own depth array, feeding a
DepthAccum, a BamStatAccum and a PreseqAccum from the same record stream, so
the alignment is read once. A separate pass over unmapped records feeds the
counters flagstat and idxstats report. Workers run longest contig first and
their number is bounded by --max-depth-workers, defaulting to a 4 GB budget
divided by the largest contig, because each worker costs four bytes per base.

Outputs land under mosdepth/, samtools/ and preseq/, or flat with
--flat-output. Input without duplicate marks is rejected unless
--skip-dup-check is passed.

Also fixes the samtools stats header, which hardcoded "rustqc rna" and so
labelled DNA output as RNA output.

End-to-end parity tests run the binary and compare against the fixtures:
all six mosdepth files match byte for byte, flagstat and idxstats match
exactly, and all 1889 data lines of samtools stats match. The stats header
differs by design, RustQC naming itself rather than reproducing samtools'
version banner, so that comparison is on data lines.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
InputSummary gains an optional dna block carrying genome length, covered
bases, mean, median and maximum coverage, the percentage of the reference at
or above each requested threshold, and the duplicate rate. An input carries
either the RNA fields or this one, never both.

Coverage thresholds are a list of objects rather than a map so the requested
order survives serialisation; a map keyed by the threshold would sort "10"
before "5".

CITATIONS.md for a dna run cites mosdepth, samtools and preseq, and none of
the RNA-only tools. The header is now shared between both writers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n used

Closes two gaps left open in this PR.

The bgzf BED outputs now get a .csi index built through htslib's
tbx_index_build, as mosdepth writes and as tabix needs to seek into them. CSI
rather than TBI because CSI carries no 512 Mb coordinate ceiling.

Indexes are not compared byte for byte: an index is binary metadata over the
compressed blocks, and two writers answering the same queries need not produce
the same bytes. The test asserts instead that a region query returns the same
rows through our index as through mosdepth's, going through the tabix binary
because rust-htslib's tabix reader ends a fetched region with a
TabixTruncatedRecord rather than stopping, and does so at different points for
the two files. It skips where tabix is absent.

CITATIONS.md for a dna run now cites samtools v1.24, the version its fixtures
were generated with, instead of the v1.22.1 the rna pipeline was validated
against. Each pipeline cites the version it was actually compared with rather
than both claiming the newer one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Matches Picard 3.4.0 byte for byte on the project fixture, metrics row and
all 170 histogram lines.

Every rule was measured against Picard's own output rather than recalled. The
inclusion filter is paired, not secondary, supplementary, duplicate or
unmapped, mate mapped, and a positive TLEN so each pair counts once. Proper
pair is deliberately not required: requiring it drops one pair and shortens
the maximum from 300 to 239 on this data.

Mean and standard deviation are over the histogram trimmed to DEVIATIONS
median absolute deviations either side of the median, with the n-1
denominator; minimum and maximum are over the untrimmed set. WIDTH_OF_XX
grows a window symmetrically around the median until it covers the
percentile, reporting 2i+1; all eleven widths match.

The fixture does not exercise trimming, since nothing on it lies beyond ten
MADs of the median, so that path has its own unit test.

Fixtures are generated with the JVM locale pinned to English: a French
default writes "3,531312" where an English one writes "3.531312", which would
make them depend on the machine that produced them. Picard's four-line
preamble is stripped, holding only a command line and a timestamp.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Matches Picard 3.4.0 on every column and every one of the 251 histogram
lines, with two exceptions noted below.

The exclusion model was derived by reproducing Picard's own numbers until
each fraction matched, not recalled. Unmapped, secondary and supplementary
records never enter the calculation. Every other record's reference-consuming
bases form the denominator of all PCT_EXC_* columns, 670989 on the fixture.
Exclusions then apply in order: duplicate, low mapping quality and unpaired
remove a whole read; low base quality and mate overlap remove single bases;
depth beyond COVERAGE_CAP is counted as excess. What survives is the high
quality coverage the histogram reports. SD_COVERAGE is the sample standard
deviation over every base of the territory, uncovered ones included.

This needs its own depth accumulator rather than a correction applied to the
mosdepth one, because the two tools do not agree on which reads or which
bases count. That was the design's reason for keeping the accumulators
separate and it holds up.

HET_SNP_SENSITIVITY and HET_SNP_Q come from Picard's TheoreticalSensitivity,
a Monte Carlo simulation whose draws would have to be reproduced bit for bit.
Both are written as "?", the marker Picard itself uses for a value it cannot
compute, and the parity test permits a difference in exactly those two
columns and nowhere else.

CollectWgsMetrics needs --reference to count the reference's non-N bases;
without one it is skipped with a warning rather than reported against a wrong
genome territory.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

1 participant