Option to use whole-genome ref for mgSplit - #1993
Open
glennhickey wants to merge 3 commits into
Open
Conversation
glennhickey
force-pushed
the
mgsplit-wg
branch
from
September 1, 2026 23:03
6253e89 to
8397629
Compare
--mgSplit builds each chromosome's minigraph from that chromosome alone, which loses the inter-chromosome competition the whole-genome pipeline relies on. A region homologous to several chromosomes -- the acrocentric short arms -- maps to all of them there, and rgfa-split drops the mappings whose target chromosome disagrees with the query's assignment. With only one chromosome in the graph there is nothing to lose to, so the same region aligns confidently to whichever chromosome it happened to be binned to. --mgSplitWholeGenomeRef builds the second-pass graph against the whole reference genome(s) instead, so those mappings compete and get filtered the way they did before, then prunes the off-chromosome material back out of the PAF, the _MINIGRAPH_ fasta and the published .sv.gfa.gz before cactus-align. Most of that was already written: separate_ref_contigs runs rgfa-split over each bin and drops what doesn't fit, and only early-returned because every chromosome's bin held a single reference contig, which the whole reference stops being true. What is new is bin selection -- keep this chromosome's bins, not every non-ambiguous one -- and rebuilding the two graph-derived artifacts off the pruned GFA. The option implies --mgSplit, since it only elaborates that pass. Every --reference is substituted, matching the first pass: --refOnly builds that graph from all of them, and it is the graph the chromosome bins were decided against. Only reference[0] is rank-0, but a secondary reference still carries sequence the primary lacks and minigraph maps against the whole graph, so it contributes competition for exactly the material this is here to catch. The substitution reaches only the graph construction. Mash sorting and last-training -- and the size estimate for last-training's own job -- keep the chromosome's own reference slice, since last_train() requires its partner sequence to exceed half the database it trains against, so a whole-genome reference would have quietly dropped every chromosome to default scores. The construction job itself is deliberately sized off the substituted map, which is why the batch memory multiple is suppressed for it. Two sizing consequences. minigraph_map_all anchors its estimate on the query, which holds while the graph is no bigger than the chromosome the query came from; this pass breaks that, so its graph term goes from 2x to 6x the (already decompressed) GFA -- measured at ~5.5x on HPRC, on top of the existing --batch 2x. It is gated on --batch as well as the option, because the option's own first pass maps whole-genome queries against a whole-genome graph, where 2x is still right. Verified on yeast: the per-chromosome .train files are byte-identical to a plain --mgSplit run, and the existing testYeastPangenomeSplitLocal bounds still hold. That test now runs this option rather than plain --mgSplit -- it covers everything the plain path did and adds the substitution and the prune -- with one assertion that each published per-chromosome graph kept only its own reference contig, which is what separates the prune working from the prune silently doing nothing. On a single-contig reference separate_ref_contigs bypasses and the substituted map equals the slice, so the option is a no-op there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XScUpHRSBsui9Y67uQxA6a
contig_sizes_for_event sets disk but no memory, so its children take Toil's 2GiB default. samtools faidx needs almost none of that -- the successful jobs report 2.5-2.7MiB -- but the job stages the whole sanitized fasta first, and cgroup accounting charges that page cache to the job. On a 3GB genome that is enough to trip the limit: one of 459 of these died with Job failed with exit value 253: 'contig_sizes_for_event' ... Exit reason: MEMLIMIT on an HPRC run, and only survived because --doubleMem retried it at 4GiB. Size it off the fasta the way sanitize_fasta_header already does for the same file, and keep the old 2GiB default as a floor -- scaling alone would ask a yeast-sized genome for 23MiB, less than the worker itself needs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XScUpHRSBsui9Y67uQxA6a
The two merges in minigraph_map_all took the 1-core, 2GiB-disk defaults, so the GAF one bgzipped single-threaded and used 37.6GiB against its 2GiB request on a 458-genome run. Their inputs are promises at graph-build time, so a small merge_pafs_sized wrapper measures them once resolved and passes disk plus the mapping cores. The merged file is also named now, instead of the anonymous .tmp that was all the bgzip log line showed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XScUpHRSBsui9Y67uQxA6a
glennhickey
force-pushed
the
mgsplit-wg
branch
from
September 2, 2026 11:16
ac285bb to
9b0ce2d
Compare
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
1 participant
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.
--mgSplitbins everything into chromosomes using the reference(s). Then does everything independently after that. The issue is that sequence that might map to other chromosomes (happens especially in acrocentrics) doesn't carry a signal after splitting.This PR adds
--mgSplitWholeGenomeRef. This does--mgSplitbut carries the whole reference into each chromosome. More expensive and annoying, but you get a proper mapq for the accrocentrics. Seems to work as designed, but still pending a full hprcv2 eval.