Bremer (decay) support via converse-constraint + pool#270
Open
ms609 wants to merge 24 commits into
Open
Conversation
Add standalone Bremer(tree, dataset, method=c("constraint","pool")) computing the
per-clade decay index L(-C) - L*, plus SuboptimalTrees() and per-tree pool scores
on MaximizeParsimony(collapse=FALSE) for landscape analysis.
Rigorous default engine forces each clade absent via new C++ negative-constraint
machinery (ConstraintData.neg_*, add_negative_constraint, displays_forbidden_clade)
plumbed through consNegSplitMatrix + an internal .negativeConstraint arg. Soundness
+ optimality via three layers: a TBR regraft move-guard, a TreePool backstop
(set_forbidden rejects any clade-displaying tree), and a not-C start repair. Drift
and annealing accept worse moves through an unguarded path, so they are disabled in
converse searches; ratchet/sectorial/nni-perturb re-optimise through the guarded
TBR and stay on for search power. Oracle-validated against exhaustive enumeration
of all binary trees, including the adversarial case (optimum displays C, not-C 3
steps away) and implied weights.
Docs regenerated: adds Bremer/SuboptimalTrees to the split-support family and
syncs the previously-stale SearchControl.Rd in-sector-drift params.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adversarial review (two background red-team agents) of the Bremer implementation surfaced ten findings; this lands the fixes. Parallel fan-out follows separately. R (Bremer.R / SuboptimalTrees.R / MaximizeParsimony.R): - BR-1 (P1): guard against a scoring-units mismatch. A multiPhylo whose stored optimal score was measured under different scoring args (e.g. trees found under implied weights, then Bremer() called with the default equal weights) silently subtracted an IW optimum from EW lengths. Re-score the reference under the active scoring args (resolving collapsed polytomies) and error on a mismatch, or on a supplied optimalScore longer than the reference tree. - BR-2 (P2): drift / in-sector drift / annealing are unsound in a converse search; a user value no longer re-enables them (was modifyList, silently emptying the pool -> all NA). Ignore with a warning. - BR-6 (P3): verify in R that each converse-search tree really lacks the clade, surfacing an engine regression as a visible NA rather than a deflated decay. - BR-3 (P3): SuboptimalTrees() strips a colliding control=/collapse= from ... (was a hard "matched by multiple actual arguments" crash on the pool path). - BR-5 (P3): carry the censored flag into format="character" output. - BRM-2 (P2, latent): MaximizeParsimony() forces nThreads=1 when a negative constraint is set (the parallel pool has no soundness backstop). C++ (ts_tbr.cpp / ts_driven.cpp), all gated on neg_active so the mainline path is byte-identical (confirmed: 180 constraint/driven/parallel/sector/fuse/ratchet/ wagner test blocks unchanged): - BRM-1 (P2): the outer-reroot sweep (try_root_edge_moves / exact_verify_sweep) was unguarded, so a root-edge TBR move could carry the search into forbidden space and inflate the reported decay (or spuriously NA). Snapshot the clade-free tree and restore+stop if the improved tree displays the clade. - BRM-3 (P3): gate the periodic-fuse improvement bookkeeping on the actual pool insertion, so a backstop-rejected fused tree no longer resets the stopping rules. - BRM-4 (perf): skip the positive post-hoc block when there are no positive splits (a negative-only constraint), removing a redundant compute_node_tips sweep. Tests: NA/bgs enumeration oracle for the DEFAULT scoring mode (previously unvalidated); BR-1 and BR-2 regressions; de-vacuumed the cross-engine test (it had been comparing empty vectors). All oracle checks (EW / IW / bgs / adversarial) pass; broad regression 180 blocks / 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`Bremer(method = "constraint")` runs one independent converse search per clade; its dominant cost is that N-search sequential fan-out. Add a `cl =` argument distributing the per-clade searches over a `parallel` cluster (each worker runs one serial search -- the negative-constraint pool guard is serial-only, so parallelism must be across clades, never within a search). The fan-out primitive `.BremerConverseScores()` seeds each clade independently (seeds drawn once from the caller's stream), so a parallel run reproduces a serial one under the same `set.seed()`, regardless of task scheduling. The default `cl = NULL` keeps the original serial path bit-for-bit; `method = "pool"` ignores `cl` (with a warning). Validated against the exhaustive enumeration oracle both serially and over a live 2-worker PSOCK cluster (identical results); the public `Bremer(cl =)` matches the serial path exactly. Adds `parallel` to Imports; completes the (previously stale) Collate with Bremer.R / BremerParallel.R / SuboptimalTrees.R. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A 3-agent adversarial pass over the red-team fix diff surfaced four issues (the C++ changes came back clean): - VF-1 (P2): the BR-1 scoring guard could raise a SPURIOUS fatal error on the canonical Bremer(trees, ds) call. It compared the stored L* to the reference's length under the current scoring, resolving collapsed MPTs with multi2di(random=FALSE); but an arbitrary resolution can be suboptimal, so min(refLen) > L* whenever poolMaxSize truncation leaves no cleanly-resolving member. Since TreeLength rejects non-binary trees, refLen can only be an UPPER bound on L* (exact L* is NP-hard). Reworked to a SOUND one-sided error (supplied L* exceeds an achievable length -> definitely wrong; multi2di inflation can never trigger it) plus a heuristic WARNING for the opposite direction (an implied-weights optimum scored under equal weights), which no sound score comparison can catch. - VF-2/VF-3: documented that a parallel run is reproducible under set.seed() but, because its per-clade seeding differs from the serial stream, may differ from a serial run where a search does not saturate (both valid upper bounds); added a public Bremer(cl=) vs serial equality test on saturating data (the primitive tests only covered .BremerConverseScores). - VF-4: the aggregate NA warning no longer misattributes a "returned tree still displays the clade" NA (whose per-clade warning is lost on cluster workers) to a search-budget shortfall. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…the forbidden clade The reduced-dataset sector solve (rss_search/xss_search) and the prune-reinsert backbone TBR run constraint-blind, so a reinsertion could rebuild a forbidden clade even though the pre-move tree lacked it. The pool backstop still guaranteed soundness (a clade-displaying tree is rejected at pool insert), but the LIVE search tree could strand on the clade with no improving escape move -- wasting the converse replicate and inflating/NA-ing the reported decay on datasets with >= 12 tips (where sectors engage). This is structurally invisible to the <= 7-tip enumeration oracle, so the gap was never caught. Mirror the positive post-hoc constraint check with a negative displays_forbidden_clade revert in rss_search/xss_search and the prune-reinsert accept path, gated on neg_active so mainline (positive/unconstrained) searches are byte-identical. Also fail loudly on a consNegSplitMatrix column-count mismatch (previously a silent unconstrained converse search -> decay reported as 0) and correct a misleading displays_forbidden_clade doc comment. Red-team round-2 findings A-11/A-12/A-15/A-17. Validated: mainline sector/prune-reinsert/positive-constraint/driven suites unchanged (613 assertions), <= 7-tip enumeration oracle exact, and a new >= 12-tip sector test asserts the converse trees genuinely lack the clade. Reach-benefit magnitude is a Hamilton follow-up (not measurable on the exhaustive oracle). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rip pool-size overrides Three red-team round-2 R findings: - optimalScore = NA_real_ slipped past the is.null() "not supplied" sentinel and crashed the one-sided scoring guard with "missing value where TRUE/FALSE needed"; now rejected up front with a clear message (B-8). - Bremer(format = "character") dropped the `censored` attribute entirely when no clade was censored, while the numeric format kept an all-FALSE vector -- so downstream code assuming its presence broke on the character path whenever nothing happened to be censored. The character format now always attaches a node.label-shaped censored vector (B-9). - SuboptimalTrees() stripped colliding `control`/`collapse` but not the raw `poolSuboptimal`/`poolMaxSize` SearchControl fields, which would silently win over the values set from `maxSuboptimal`/`maxPool` via MaximizeParsimony()'s dots-override-control merge; now warned and ignored (B-10). Tests added for each; Bremer / SuboptimalTrees / BremerParallel suites green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…a fuzzy error The scoring-units guard now checks a supplied optimalScore against the reference's length under the scoring arguments in effect and WARNS (in either direction) on any material difference, then accepts the score and proceeds -- per the package author's decision to trust the user's scoring choice and only alert them in case they forgot to pass the scoring arguments used to find the trees. This replaces the previous one-sided 5%-of-length heuristic, which had a P1 blind spot: two criteria that place L* close together (a finder reproduced inapplicable="bgs" vs "missing" giving an identical length) slipped under the threshold and produced a silently mis-scored decay vector. The tight two-sided tolerance now catches a small but genuine mismatch; taking the minimum over the supplied MPTs absorbs multi2di resolution slop, so a consistent call does not false-warn (verified across the EW / IW / bgs test corpus). The former hard error on an "impossible" L* is downgraded to the same accept-and-proceed warning. Red-team round-2 finding B-7 / TA-6. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…check it exactly in Bremer A saved optimal score is only meaningful alongside the conditions it was optimal under. MaximizeParsimony() now stamps a `scoring` attribute -- a canonical signature of the scoring arguments (concavity, extended_iw, xpiwe_r, xpiwe_max_f, hierarchy presence, inapplicable, hsj_alpha) -- on its returned multiPhylo. Bremer() compares that signature EXACTLY against the scoring arguments in effect: a mismatch (e.g. implied-weights trees passed to a default equal-weights Bremer) warns and proceeds (trusting the user's choice); a match is silent. This closes the B-7/TA-6 blind spot at the root -- it needs no re-scoring, so it has neither the resolution slop nor the near-equal-length hole of the length heuristic, which is retained only as a fallback for a bare optimalScore or a reference built outside MaximizeParsimony. Additive attribute; validated across EW / IW / profile / hsj / xform / hierarchy modes (537 assertions in MaximizeParsimony / Jackknife / scoring-mode suites unchanged) and the Bremer suite (provenance match = no warning, mismatch = warning, length fallback intact). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lignment Two of the deferred round-2 test-adequacy gaps, added with fast searches: - TA-9: Bremer() on a fully-unresolved (star) reference warns and returns an empty numeric (the "nothing to calculate" early return, previously untested). - TA-5: SuboptimalTrees() under a tiny maxPool exercises the diversity-eviction path and asserts the `scores` attribute (and each tree's `score`) stays aligned with the evicted-down set -- the exact truncation mechanism that hid VF-1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The aggregate-NA warning (VF-4) and the per-clade "returned tree still displays the clade" check (BR-6) guard against engine states the C++ move-guard + pool backstop are designed never to reach, so an ordinary search cannot provoke them. Add a proper test seam instead of leaving them untested: - Extract .BremerProcessResult() -- the per-clade result handler -- and unit-test its NA sentinels (non-finite / negative score) and its displays-clade warning path directly (TA-4 / BR-6). - Add an internal .runConverse injection argument to .BremerConstraint() so a stub engine can drive every clade to NA, exercising the aggregate NA warning and the NA assembly end-to-end (TA-3 / VF-4). Pure refactor of the real path (processConverse now delegates to the extracted helper); the full Bremer + BremerParallel suites are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Brings the Bremer branch up to current cpp-search, notably the native inapplicable kernel replacing Morphy (#251) and the FixedTree change. Conflict resolutions: - R/MaximizeParsimony.R: keep the scoring-signature helpers (.ScoringSignature / .ScoringSignatureMatch / .DescribeScoring) added for Bremer provenance; honour cpp-search's removal of the deprecated MaximizeParsimony2 alias and its EasyTrees launcher. - DESCRIPTION Collate / NAMESPACE / man: regenerated with roxygen; drop Sectorial.R (deleted upstream) and C_MorphyLength (Morphy removed), keep SuboptimalTrees + Bremer exports. - RcppExports regenerated: Morphy exports dropped, negative-constraint plumbing retained. Also files SuboptimalTrees() under @family tree scoring (in addition to split support functions). Bremer, SuboptimalTrees and constraint suites: 126 pass, 0 fail. Full recompile of the merged C++ is clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The sense-check run on the PR surfaced 2 ERRORs + 2 WARNINGs; all four are doc/hygiene, not code: - examples ERROR: the Bremer example called LabelSplits() with no active device (edgelabels -> `last_plot.phylo` not found), and keyed decay onto trees[[1]] rather than the consensus it is computed against. Plot and label the consensus reference so node numbers align and a device exists. - tests ERROR (spelling): add bgs, bipartitions, clade's, run's, search's to inst/WORDLIST (new terms in the Bremer/SuboptimalTrees docs). - Rd WARNING: document the internal .negativeConstraint argument of MaximizeParsimony() (roxygen requires every \usage arg documented). - tests WARNING: declare pkgload in Suggests (test-BremerParallel.R uses it for the PSOCK worker load_all()). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The R 4.1 CI leg hung 81 min (-> job cancelled) in the Bremer `\donttest` example, while the same example completes in ~40s on the release/devel legs. Root cause is not slowness (4x would be ~3 min): the example had no `set.seed()`, so each leg's RNG stream yields a different consensus/clade set, and on R 4.1 one clade's converse-constraint search (method="constraint" fans out to one search per clade, ~15 here) fails to terminate under the default unbounded budget. Make the executed example structurally unable to hang: - `set.seed(0)` for a reproducible search. - Run the fast `method = "pool"` path (a single bounded SuboptimalTrees search, no converse fan-out) as the `\donttest` demo. - Demote the rigorous default `method = "constraint"` to `\dontrun`, and model `maxSeconds` there as the recommended bound. Bremer's correctness is covered by the oracle + 126 testthat assertions, so the example is illustrative, not test surface. The underlying converse-search non-termination under an unbounded budget is tracked separately (non-blocking). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The macOS-latest R-CMD-check leg hung ~6h on `checking examples with --run-donttest` while windows-latest -- the SAME `r: 'release'`, so the same `set.seed(0)` R-level RNG stream -- ran the whole donttest set in 46s. Same seed + same R version rules out RNG divergence or a data-dependent search non-termination; the only differences are OS/threading, i.e. a platform-specific parallel deadlock (cf. the MinGW emutls / parallel-NA hang class). `set.seed()` fixes R's RNG but not the C++ worker-thread RNGs, so a threaded search is neither reproducible across platforms nor free of the deadlock path. Pin the illustrative example to `nThreads = 1`: this seeds the C++ search from R's main-thread RNG (fully reproducible) and removes the parallel path entirely. The example demonstrates the API, not parallelism, so nothing of value is lost. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Serializing only the Bremer example did not clear the macOS-latest 6h donttest hang. The next `\donttest` example alphabetically is SuboptimalTrees (also new in this PR), whose example ran a default-threaded 23-taxon search with no seed -- the same macOS parallel-deadlock exposure. Pin it to `set.seed(0)` + `nThreads = 1` like the Bremer example, so every donttest example this PR adds is serial and reproducible across platforms. If macOS still hangs after this, the remaining suspect is the pre-existing `TaxonInfluence` donttest example (threaded, one search per dropped taxon), which is outside this PR's scope. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…hang
The `checking examples with --run-donttest` step burned the 6 h CI ceiling on
whichever core leg was slowest (macOS one run, ubuntu R 4.1 the next). Root
cause: the Bremer `method = "pool"` example called
Bremer(trees, dataset, method = "pool", nThreads = 1)
which routes to .BremerPool -> SuboptimalTrees(dataset, maxSuboptimal = 10, ...)
forwarding NO maxReplicates and NO maxSeconds. That runs the full default
budget -- 96 replicates of a depth-10 suboptimal-pool search -- with the C++
wall-clock timeout DISABLED (use_timeout is false whenever maxSeconds == 0), so
nothing caps it. `nThreads = 1` dispatches the serial `driven_search`
(ts_rcpp.cpp:1868), so this is not the parallel-teardown deadlock class: it is a
genuinely heavy, uncapped search whose wall time straddles the 6 h limit and so
hangs whichever platform is slowest. cpp-search never had this example, which is
why the hang is new to this PR.
Fix: bound the example's pool call with `maxBremer = 3` (pool depth),
`maxReplicates = 4`, and a `maxSeconds = 20` safety cap, and document why the
bounds matter. The runnable preview now completes in seconds on every platform.
Also add .github/workflows/donttest-probe.yml: a fast (25-min-capped) lane that
installs the branch and runs the suspect donttest examples directly under
`timeout` on the two surfaces the full check hangs on (macOS-latest and the
ubuntu R 4.1 leg), so a runaway example fails in minutes with a located cause
instead of burning 6 h. It also times the uncapped pool search per replicate to
evidence the heaviness.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…amples example() run standalone has no graphics device on a headless runner, so the Bremer example's plot()/LabelSplits() would error spuriously; R CMD check provides one automatically. Open pdf(NULL) to mirror that. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dows The first probe measured the wrong kernel: Bremer(method="pool") defaults to inapplicable="bgs" and forwards it into the pool search, but the diagnostic called SuboptimalTrees without inapplicable -> default kernel (4s/rep, fast). Rebuild the probe to (a) run the default vs bgs kernel head-to-head, (b) run the verbatim unbounded Bremer(method="pool") repro, both under a portable timeout (gtimeout via coreutils on macOS; timeout on Linux), and (c) add windows-latest as a determinism cross-check (same seed/R as macOS but passes the full check). Fixes the exit-127 macOS failure (macOS has no `timeout`). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…er trail Bremer is exonerated (unbounded pool runs <9s on macOS+ubuntu, byte-identical). The hang is PR-introduced yet every example is fast in isolation, so test the untested condition: all \donttest blocks concatenated in ONE R process (as R CMD check runs them), looped, with a durable marker trail, under gtimeout. The new serial searches run before TaxonInfluence's parallel (default-nThreads) search; a hang's last START marker localises the culprit block. A solo TaxonInfluence loop distinguishes an interaction from a standalone parallel hang. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…te discriminator Concatenated repro pinned the hang to the TreeLength block (macOS iter2, ubuntu iter1; windows never). Now inline TreeLength's three sub-calls (profile/random5/hsj) with a marker + .Random.seed digest before each, primed by the real Bremer/SuboptimalTrees/TaxonInfluence sequence and looped, so the last START marker names the hanging call and the seed digest rules RNG in or out. An unprimed control establishes whether the hang requires the accumulated C++ state the bgs/NA searches leave behind. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Hang localized to the pre-existing TreeLength(concavity='profile') call on Agnarsson2004 (62t/225ch); profile code is byte-identical to cpp-search (full diff). Time StepInformation per unique character pattern at n_mc=1000 then 100000 with START markers, so macOS's last START names the hanging pattern and n_mc-scaling separates MC-perf from exact-recursion. Co-Authored-By: Claude Opus 4.8 <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.
Bremer (decay) support
Adds Bremer support calculation to TreeSearch, plus the suboptimal-tree
infrastructure it builds on. Bremer support for a clade C is
L(¬C) − L*— the extra tree length required before C stops appearing onoptimal trees.
What's new
Bremer(tree, dataset, …)— a standalone post-search function (mirroringJackLabels()), returning a decay index per resolved clade, keyed by nodenumber.
method = "constraint"(default, rigorous). One converse-constraintsearch per clade, forcing that clade absent and taking the shortest
resulting tree. TNT/PAUP*-grade, bounded memory (best tree per clade).
method = "pool"(fast preview). Min length among retained suboptimalpool trees lacking each clade — an upper bound, right-censored at the
sampling depth (
attr(, "censored")).TreeLength()/MaximizeParsimony(), so Bremer iscomputed under exactly the search's optimality criterion (equal weights,
implied weights, profile, inapplicable "bgs" — fractional decay indices are
preserved, never coerced to integer).
parallelcluster via
cl =for near-linear speed-up on many-clade trees.SuboptimalTrees(dataset, …)— returns every tree the search retainedwithin a given number of steps of the optimum, each annotated with its
parsimony score, for landscape analysis.
MaximizeParsimony(collapse = FALSE)now surfaces the retained pool's per-tree scores via a
scoresattribute (anda
scoreon each tree), soSuboptimality()works on the result directly.Scoring provenance.
MaximizeParsimony()now records the optimalitycriterion it searched under as
attr(result, "scoring").Bremer()checks itexactly: if a supplied
optimalScore/reference was found under a differentscoring mode, it warns and proceeds (trusting the user's declared mode) —
a saved optimal score is only meaningful alongside the conditions it was
optimal under.
Correctness
exhaustive, so
method = "constraint"is asserted equal to the exactmin(len[¬C]) − min(len)overAllTrees, including an adversarial case wherethe unconstrained optimum displays C and the ¬C optimum is several steps
away.
per-candidate regraft filter (mirroring the positive constraint), with a pool
set_forbiddeninsert-time backstop and reverts in the sector /prune-reinsert paths so a converse search can never rebuild the forbidden
clade.
(r-hub container, R-devel ASAN/UBSAN) run on the merge tip
b26f948c:AddressSanitizer examples / vignettes / tests all pass
(run 29083961613, ~2h23m).
Tests
test-Bremer.R,test-BremerParallel.R,test-SuboptimalTrees.R— oracleequality (equal-weights, implied-weights, inapplicable), cross-engine bound
(
constraint ≤ pool), star-tree / zero-clade early return,optimalScorevalidation, scoring-provenance match/mismatch, pool eviction-score alignment,
and engine-failure paths via an injection seam. Local Bremer + SuboptimalTrees +
constraint suites: 126 pass, 0 fail.
Merge note
This branch is merged up to current
cpp-search, including the nativeinapplicable kernel replacing Morphy (#251). The one substantive conflict
(
MaximizeParsimony.R) was hand-resolved; NAMESPACE / Collate / man /RcppExports were regenerated with roxygen +
compileAttributes(Morphy exportsdropped, negative-constraint plumbing retained).
Deferred (non-blocking)
Cross-process PSOCK fault-injection tests (worker value-correctness under
mis-serialized scoring args) and a Hamilton A/B on the converse-search reach
benefit remain flagged; neither gates correctness.
🤖 Generated with Claude Code