feat: add ft union-peaks to merge peak calls from many BED files - #129
Conversation
Adversarial review + scale test15 agents across 4 lenses (correctness, refactor safety, CLI/docs, genome-scale perf), each finding independently refuted before being kept. 10 candidate findings, 9 refuted, 1 survived. Fixed in 8980f7fPer-island interval selection rescanned every interval of every sample for every island —
Output byte-identical before/after on all three benchmark sets (genome: 2,071,199 peak rows). Peak RSS flat at ~340 MB genome-wide — islanding is a ~1,780x memory reduction over the whole-chromosome pileup it wraps. Also corrected four docs the review found overpromising: islanding bounds the rolling-max failure mode but is not identical to a whole-chromosome run; Refuted (checked, not real)
Refactor safety was re-checked on the paths the original gate did not cover (FDR mode, |
|
Depends on #130. Rebase this branch after #130 merges. Why it depends. Both branches change the same three files: Two things to check after the rebase.
|
18a5e2b to
7a22a6a
Compare
Running mock-fire and call-peaks by hand needs an intermediate coordinate sort and index, which mock-fire cannot produce (it writes in read-name order), so combine the two into one command that takes N BED files samtools-merge style. Each input BED is one sample: its intervals become FIRE elements on mock fibers and go straight into the peak caller in memory. Peaks are reported with the number and names of the input BEDs that support them. To share the peak caller, extract `call_peaks_for_chrom` out of `call_peaks` and move the CLI values it reads into `PeakCallingParams`, so a command that never opens a BAM can call it (`CallPeaksOptions` flattens `InputBam`, whose fields also carry env-var defaults that would leak in). `ft call-peaks` output is byte-identical before and after on ctcf.bam, all.bam, and NAPA.bam. The window is sized to islands of input intervals instead of whole chromosomes: the pileup track costs ~56 bytes a base, which is tens of GB genome-wide.
Rescanning every interval of every sample for every island cost islands x intervals per chromosome, which was ~77% of the run on dense input. The intervals are already merged and sorted, so their ends rise with their starts: seek to the first one reaching the island and stop at the first one past it, the way support_for already does. 20 samples x 100k intervals on one chromosome (100k islands): 151.8s -> 19.3s. Whole-genome (20 x 150k peaks, hg38): 269.9s -> 237.9s, RSS flat at ~340 MB. Output byte-identical on all three benchmark sets. Also correct the docs the review found overpromising: islanding bounds the rolling-max failure mode but is not identical to a whole-chromosome run, peak_summit can fall outside start/end, only local maxima become peaks, and --min-support renumbers the sequential name column.
Measured islanded output against a whole-chromosome mock-fire | sort | index | call-peaks run over 60 adversarial configurations: 11 summit shifts, 1 peak-set change. The shifts are tie-breaks inside a plateau of equal sample support, and the support columns never come from the pileup.
7a22a6a to
ab58d99
Compare
call_peaks_for_chrom is now also union-peaks' engine; the snapshot makes the byte-identical refactor gate permanent.
ft union-peaks replaces the mock-fire | sort | call-peaks pipeline. The command keeps working for existing scripts (StergachisLab fire_consensus_pipeline) but is hidden from help and warns at runtime; also updates a stale pre-callable-model comment.
- Book-ended intervals from different samples: the touching sample formed the peak but was silently dropped from support and the union span; support now counts exact abutment with the consensus core (whose bounds are medians of element edges). The consensus core itself is unchanged, so ft call-peaks output is untouched. - Coordinates past the mock-header/liftover range (int32 LN, u32 positions) error cleanly instead of vanishing with exit 0 or panicking. - Warn once when an island exceeds 10 Mb: memory scales with island size and dense input can grow islands to chromosome scale. - --min-support 0 errors instead of silently acting as 1; -n above the input count warns that no peak can be reported. - read_bed_regions skips track/browser lines, so MACS2/UCSC peak files work unedited. - Docs: within-file collapse also merges book-ended intervals; the 56 B/base figure gains its per-element term. Both fixes pinned by new regression tests.
The consensus start/end were the upper median of the pooled FIRE element edges, which lands entirely on one element's bounds whenever contributors tie (the book-ended two-sample case sat flush on the second sample). Even counts now take the midpoint of the two middle values. On the ctcf fixture 6 of 31 call-peaks boundaries shift by 1-15 bp (summits and all statistics unchanged); the union-peaks two-sample core centers between its contributors.
ceil(frac x n_inputs) folds into the same threshold, making the fire_consensus_pipeline's --min-fire-frac semantics expressible directly.
The NucleosomeParameters pattern: the peak-caller knobs live in one #[derive(Args)] struct flattened into CallPeaksOptions (flags unchanged), and union-peaks keeps constructing it directly. Deletes the 12-field From mapping that had to be kept in sync by hand.
PeakMergeParams (window size + the three merge overlaps + iteration cap) splits out of PeakCallingParams and flattens into both CLIs; defaults are unchanged on both. The mode/coverage fields stay hardcoded in union-peaks, where FDR and real-fiber coverage semantics do not apply.
Changelog restoration: the actual fix shipped inside the #129 squash (consensus start/end were the upper median of pooled FIRE element edges, landing on one element's bounds whenever contributors tied; even counts now take the midpoint of the two middle values — 10 of 31 ctcf call-peaks boundaries shifted by 1–33 bp, summits and statistics unchanged). Squashing folded it into the union-peaks feat entry, so v0.14.0's changelog had no Fixed line for it. This PR carries the doc comment for the median function and, through its squash title, restores the changelog entry. Co-authored-by: Mitchell R. Vollger <mvollger@gmail.com>
Combines
ft mock-fireandft call-peaksinto one command that takes any number of BED files and returns a single union peak set.Each input BED is one sample: its intervals become FIRE elements on mock fibers, which go straight into the existing peak caller in memory. Doing this by hand needs an intermediate coordinate sort and index, which
ft mock-firecannot produce on its own (it writes in read-name order, sosamtools indexfails withUnsorted positionswhenever read names sort against positions).Output
start/endare the peak caller's consensus (median) boundaries of the overlapping input intervals;union_start/union_endare their outer span. Overlapping intervals within one file are collapsed first, so a file can add at most 1 ton_support.--min-supportfilters the output only, so-n 3and-n 1plus a downstream filter agree.Flags:
-o/--out,--names(default: file basenames),-n/--min-support,--min-frac-support, and the shared peak-caller knobs (--window-sizeplus the merge thresholds), exposed through the samePeakMergeParamsstructcall-peaksuses, with identical defaults.Refactor
To share the peak caller,
call_peaks_for_chromis extracted out ofcall_peaksand the CLI values it reads move into aPeakCallingParamsstruct.CallPeaksOptionsflattensInputBam, which cannot be built by a command that never opens a BAM, and whoseFiberFiltersfields carryenv=defaults (MIN_MSP,MIN_AVE_MSP_SIZE,FT_MIN_ML_SCORE) that would otherwise leak into union peak calls.Peaks are emitted through a callback, so
Peakand itsDisplayimpl are untouched andft call-peaksoutput cannot drift.Gate:
ft call-peaks --min-fire-frac 0.5output is byte-identical before and after the refactor ontests/data/{ctcf,all,NAPA}.bam.The pileup window is sized to islands of input intervals rather than whole chromosomes; the track costs ~56 bytes a base, which is tens of GB genome-wide. On the test fixture, islanded output matches a hand-run
mock-fire | samtools sort | samtools index | call-peakspipeline exactly, except where that pipeline double-counts one file's overlapping intervals.Tests
5 regression tests in
tests/regression/union_peaks.rs(support counts and boundaries, within-file collapse,--min-support, names ordered against positions, empty input, error paths) plus 2 unit tests forislands/support_for.cargo fmt,cargo clippy -p fibertools-rs, andcargo test -p fibertools-rsare clean.mock-fire deprecation
ft mock-fireis now hidden from help and warns at runtime that it will be removed: union-peaks replaces themock-fire | samtools sort | call-peakspipeline (the one user found in the wild, StergachisLab/fire_consensus_pipeline, keeps working meanwhile — its--min-fire-coverage 1 --min-fire-frac 0.01maps to union-peaks' built-ins plus--min-support). The module code stays as union-peaks' engine.Refactor gate, made permanent
ft call-peaksoutput on the ctcf fixture is now snapshot-pinned (tests/regression/call_peaks.rs), so the byte-identical guarantee for the sharedcall_peaks_for_chrompath is enforced by CI rather than a one-off manual check.Deep-review outcome
A multi-agent review compared this against the legacy pipeline empirically: on
ctcf.bed.gz,mock-fire | sort | call-peaks --min-fire-coverage 1 --min-fire-frac 0.01andft union-peaksproduce identical peak sets (boundaries and summits byte-equal, legacyfire_coverage==n_supportin the multi-sample case), deterministically, at ~0.01 s vs ~10 s. Every divergence is intended and documented except two that this PR now fixes: exactly book-ended intervals from different samples no longer drop the touching sample from support (fixed at the root too: consensus boundaries now use a true median — even element counts take the midpoint of the two middle values instead of the upper median, shifting 10 of 31 ctcf call-peaks boundaries by 1-33 bp with summits and all statistics unchanged), and out-of-range coordinates (past int32 header / u32 liftover space) error cleanly instead of vanishing. Also added: a warning when an island exceeds 10 Mb (memory scales with island size),--min-supportmisuse guards, andtrack/browserline tolerance in the BED reader.Migration note for fire_consensus_pipeline: union-peaks derives the sample from the input file and ignores the BED 4th column — passing the pipeline's single
merged.4col.bed.gzgives one sample (n_support≡ 1). Split per-sample BEDs to migrate. Its--min-fire-frac 0.01maps directly to the new--min-frac-support(the fractional twin of-n; both fold into one threshold); residual differences appear only past ~100 samples (the 1% floor in the peak caller itself).