WIP: minipoa - #2010
Open
glennhickey wants to merge 5 commits into
Open
WIP: minipoa#2010glennhickey wants to merge 5 commits into
glennhickey wants to merge 5 commits into
Conversation
abPOA's memory use is what forces BAR's 10kb window. minipoa is a minimizer-based POA that partitions the graph on its consensus path and needs far less of it, so it can take a much larger window. On evolver mammals the two are equivalent on accuracy -- 0.8506 against abPOA's 0.8513, and 0.9974 against 0.9973 on primates -- at 1.0 GB peak RSS against 2.3 GB, with a 100kb window against abPOA's 10kb. abPOA stays the default. Selected with <bar baseAligner="pecan|abpoa|minipoa">. There is no command line flag; the config attribute is the only selector. Configs written before the attribute existed keep working: both it and the older <bar partialOrderAlignment> boolean are optional, and when only the boolean is present it decides as it always did. When both are present and disagree, baseAligner wins and says so. Scoring is the part that needed care. minipoa shares abPOA's substitution matrix, from <poa partialOrderAlignmentSubMatrix>, so substitutions score identically and last-train's learned matrix reaches both. The gap penalties are deliberately separate, because the same nominal values do not produce the same alignment: abPOA's gap model is convex, min(open1 + L*ext1, open2 + L*ext2), so with the shipped 400/30 and 1200/1 its effective extension past L~28 is 1, not 30. minipoa has a single affine piece, and charging it abPOA's first-piece 30 prices long gaps about 30x above what abPOA charges. It responds by packing bases into shared columns instead of opening a gap -- 9% fewer columns and 151k spurious aligned pairs measured across 229 real BAR windows -- which cost about six points of mafComparator accuracy. That matters here because 94% of the gap bases in the evolver mammals truth sit in runs of 28bp or longer; on data with mostly short gaps, such as human pangenomes, the two models differ far less. <minipoa minipoaGapOpenPenalty/ minipoaGapExtensionPenalty> default to 600/4, which are fitted to that test and worth revisiting against other data. --lastTrain replaces them and is a better fit for minipoa than for abPOA: it trains a single affine model, which is minipoa's model exactly, whereas abPOA has to be handed a synthesised second piece to stay stable. Build: minipoa is a submodule built into lib/libminipoa.a, gated on a "minipoa" variable in include.mk that defaults on for x86 and off for ARM, where its simde/NEON path is unproven. With it off the submodule is not built, -lminipoa is not linked, and selecting the engine aborts with a message naming the switch, so a tree that cannot build minipoa still builds cactus. -lminipoa sits next to -labpoa, ahead of -lz: libminipoa.a has undefined references into zlib and ld resolves archives left to right. Verified: 20 unit tests over both engines; abPOA byte-for-byte identical to master across 229 windows, so the refactor preserves it; SSE2 and AVX2 produce identical alignments; full static link; docker image builds and contains a working minipoa; evolver primates and mammals accuracy both pass. Two things here are independent of minipoa and could be taken separately: - CACTUS_BAR_DUMP_DIR dumps every window handed to the base aligner, with a command line that replays it. This was a commented-out #define that also deleted its own output on success and named files by a pointer that got reused, so it was unusable for looking at a window that aligned badly rather than one that crashed. - last_scoring.py passed the extension factor to apply_long_gap in both the open and extension positions, so partialOrderAlignmentTrainedGapOpen2Factor was read from the config and discarded. Both default to 3, so the shipped config is unaffected; setting them differently silently did nothing, and setting the extension factor to 1 raised an assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Everything lived in one undifferentiated file, with abPOA's parameter construction at the top, minipoa's five hundred lines further down, and abPOA's actual alignment call inlined in the dispatcher while minipoa's sat in its own function. Adding a third engine meant finding the seams again. No behaviour change. The abPOA call is extracted into run_abpoa_window so the two backends are symmetric, run_poa_window becomes a four-line switch over the engine, and the file is grouped into labelled sections: the shared alphabet and Msa handling, the shared trimming and stitching, the shared dump helper, one section per backend, the selection and dispatch, then the shared windowing, adjacency and block-extraction code. The engine-specific code is now about 270 lines for abPOA and 195 for minipoa against roughly 1250 shared, which is the honest ratio -- most of BAR's base-alignment logic is engine-neutral and always was. Verified as a no-op: no line was added or removed except section banners, and both engines produce byte-for-byte identical MSAs across 229 real BAR windows before and after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The 100000 window was picked from the plan text before anything was measured, and evolver could never have caught it: BAR adjacencies there have a median length of 29bp, so no window setting between 10kb and 1Mb changes a single alignment. A random sample of 6000 dumped windows from an evolver run found zero that reached even the 10kb cap. Measured on human/chimp chr10 instead, where BAR is 85% of the runtime and adjacencies are real, at 8 cores: window 10kb 15kb 20kb 30kb 50kb+ peak 3.5G 5.9G 9.3G 15.1G OOM-killed above 23G BAR 191s 196s 197s 204s - Memory goes roughly as window x (bandConstant + bandFraction x window), so it is quadratic-ish, not flat, and 100000 is comfortably fatal. BAR time is flat across the whole safe range, so a larger window buys nothing even where it fits. Neither minimizer seeding nor a tighter band rescued 100kb -- both died at the same ceiling, because the first sequences build a full-size graph with no consensus to anchor against. So minipoa gets abpoa's window, and the claim that it "affords a much larger window" is removed from the config, the docs and bar.c. That claim was wrong: minipoa's advantage is per-window cost at the same setting -- 3.5G and 191s against abpoa's 7.9G and 221s on the same data. minipoaProgressiveMode also defaults off now. It was on to match abpoa, on the reasoning that insertion order matters for POA. It does in principle, but this guide tree has not earned its cost on anything measurable: bit-identical output on evolver, and 7% more memory and 2% more time on chr10. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One conflict, in Makefile: master added the pangenome-extend and yeast-extend test targets while this branch added the two minipoa evolver targets to evolver_test_all_local. Both kept. Master does not touch <bar> or <poa>, so there is no interaction with minipoa's scoring. Verified after merging: 20 unit tests pass and both engines produce byte-for-byte identical MSAs across 229 real BAR windows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI failed the build: CGL_DEBUG=ultra compiles with -Werror, and six sprintf(path, "%s.mat", other_path) calls between two char[1024] buffers trip -Wformat-overflow, because the compiler cannot prove the directory name leaves room for the suffix. impl/poaBarAligner.c:790:33: error: '.mat' directive writing 4 bytes into a region of size between 1 and 1024 [-Werror=format-overflow=] Both backends had their own copy of the same eight lines, so this fixes the duplication and the warning together: next_dump_paths() builds all four names once, with snprintf for the base name and memcpy for the suffixes. A dump directory too long to name a window now logs and skips the dump rather than writing a truncated path that could collide with another window's files. Local builds never caught this because they do not set CGL_DEBUG. 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
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.
Add support for
minipoaas drop-in replacement forabpoa.minipoapurports to be a bit faster and more memory efficient thanabpoa, so it could allow bigger windows to be aligned at once.For now, it's off-by-default, and only activated by changing the config. But working well enough to warrant some bigger tests...