fix: enforce user constraints at the start-tree, pool-capture and collapse boundaries - #59
Open
ms609 wants to merge 3 commits into
Open
fix: enforce user constraints at the start-tree, pool-capture and collapse boundaries#59ms609 wants to merge 3 commits into
ms609 wants to merge 3 commits into
Conversation
…lapse boundaries Three independent holes let a `constraint` stop binding the trees the caller is handed. A start tree supplied through `tree =` was never checked against the constraint (T-402). Constrained rearrangement cannot climb out of a violating tree -- an unmapped split makes every candidate regraft illegal -- so the replicate froze on it and reported its unconstrained score, which then evicted the compliant trees the other replicates found. The repair has to happen before anything takes the start's score as a baseline: a violating tree is drawn from a wider set of topologies and so scores better, making the legal repair look like a regression to any later accept test. `run_single_replicate` now repairs any violating start, whatever its source, and falls back to a constrained Wagner build where the heuristic repair does not take. `MaximizeParsimony()` warns when it was the caller's `tree` that conflicted. The per-replicate pool capture had no constraint gate, asymmetrically to the fuse capture beside it (T-324). All three capture sites -- interrupted and normal in the serial driver, and the parallel worker's -- now go through `capture_satisfies_constraint()`, which tests with `violates_constraint_posthoc` rather than the fuse's `constraint_node < 0`: a tree can map every constraint node and still fail the full Fitch check. Discards are counted and reported from the main thread. With the pool gated, an empty pool under a constraint is an error rather than a fall back to the unvalidated start tree. The collapse pass protected only a node whose tip set was the 1 group exactly (T-403). Tips coded `?` for a constraint character are free to sit on either side, so the split is often realised by a wider node -- left collapsible, and contracted away under the default `collapse = TRUE`. `consZero` is now plumbed through `.PrepareConstraint()` to the kernel, which additionally protects the MRCA of either group when it excludes the other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`Resample(constraint =, nReplicates > 1)` errored: `R/Resample.R:372` splats `.PrepareConstraint()`'s whole list into `ts_parallel_resample()`, which has no `consZero` formal. It was the one splat site of four not filtered, and no test covered a constrained `Resample()` at all; a test now asserts the filter against every flat kernel's formals. The collapse protection over-reached. It protected the MRCA of a group unconditionally, so an unsupported branch that merely happened to be the narrowest node containing the 0 group was returned resolved -- the "unsupported non-constraint branches still collapse" half of the promise, and a shift in `n_topologies`. It now protects only when no realising edge survives the contraction on its own. A discarded replicate no longer feeds the strategy bandit, the Chao1 coverage scores or the replicate report: a violating tree scores better than any legal one, so those would be credited to whatever produced it. Its stopping rules still run -- skipping them would outlive the deadline and swallow an interrupt. An interrupted replicate is now tested but not repaired, since `impose_constraint()` has no interrupt check of its own. The constraint check short-circuits on the locked-node mapping, which is cheaper than the post-hoc Fitch check and strictly stronger, so only an unmapped split pays for the latter. Constrained wall-clock on Vinther2008 (30 paired seeds) goes from a 1.12 median ratio, 11 seeds >10% slower, to 1.003 with none >10%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Self-review of the two commits before it. A constraint is enforced as a split throughout -- the locked-node filter screens rearrangements on it, and impose_constraint() repairs to it and nothing else -- so a character with a third state has taxa nothing constrains. The capture gate was judging that same tree by the stricter full-Fitch reading, which is not a standard the search can reach: a probe on a three-state constraint discarded all four replicates and errored with an empty pool, where a partial answer existed. The gate now follows the mapping, and `.PrepareConstraint()` warns at input that an intermediate state is unconstrained, rather than leaving the caller to infer from `@param constraint` that it is not. `constraint_satisfied()` refreshes the DFS timestamps alongside the node ids. map_constraint_nodes() alone left the two out of step, and spr_search() reads both without re-mapping, so the `sprFirst = TRUE` warmup could classify a regraft against this tree's nodes and another tree's timestamps. A start whose repair fails falls back to a Wagner build, which is exactly the constructor that can exhaust its reshuffles and return a violating tree; it is now repaired rather than trusted. Also: the collapse protection's group sizes are counted once instead of per tree; the kernel-formals test now asserts that no constraint field is dropped, not just that none is unknown; `.ConstraintViolated()` indexes nodes by column so its accumulation runs down a column-major matrix rather than across it. Constrained wall-clock on Vinther2008 (20 paired seeds) is 0.95 median against `cpp-search`, scores identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 5, 2026
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.
Fixes #18
Fixes #1
Fixes #19
Three independent holes let a
constraintstop binding the trees the caller is handed. They are one PR because #18 and #1 share the ungated pool capture verbatim, and #19 edits the same.PrepareConstraint()return andts_collapse_poolsignature that #18 touches.What each fix does
#18 (T-402) — the start tree.
run_single_replicatenow repairs any constraint-violating start before anything takes its score as a baseline, falling back to a constrained Wagner build (itself repaired) when the heuristic repair does not take. Both documented mis-patch traps are structural consequences of one fact: a violating tree is drawn from a wider set of topologies than a legal one, so it scores better and is an unbeatable baseline for any later accept test. Acting at the boundary is the only placement that works.#1 (T-324) — the pool capture. All three sites — interrupted and normal in the serial driver, plus the parallel worker — go through a new
capture_satisfies_constraint(), which repairs then verifies, mirroring the fuse capture beside it. Discards are counted per thread and warned from the main thread, sinceRf_warning()from a worker would corrupt the heap. With the pool gated, an empty pool under a constraint no longer falls back to the unvalidated start tree.#19 (T-403) — the collapse.
consZerois plumbed through.PrepareConstraint()to the kernel, which now also protects the node realising a split when no realising edge survives the contraction on its own. Protecting unconditionally would force resolution of branches the constraint does not ask for — the "unsupported non-constraint branches still collapse" half of the promise.Repair vs reject on #18
I repaired in C++ and warned in R, rather than rejecting the start.
constraintis a guarantee about output;treeis a hint about where to start. When they conflict the guarantee has to win, but discarding the hint throws away the user's basin — and the motivating case in?MaximizeParsimonyis warm-starting from previously found optima, exactly where that basin is the point. A few SPR moves preserve it. The conflict is still the caller's to know about, soMaximizeParsimony()warns: either the tree or the constraint is not what they meant.The warning uses the phyDat reading
@param constraintdocuments (?taxa free on either side), which is a true statement about the user's contract. The repair triggers on the same reading.Pre-fix failure output
Against a clean
git archivebuild of99b79d8f,tests/testthat/test-ts-constraint-holes.Rfails 14 assertions:The constrained optimum is 10; 6 is the unconstrained optimum, which is what the frozen search reported. Line 128 is the pool test: 84 of 85 retained trees complied, one did not. Line 202 is the collapse test, which returned
(a,(e,(c,d,g,h,(b,f))))— no split separating{a,b}from{c,d}.After the fix: 35 pass, 0 fail. Full suite 11748 pass, 0 fail.
A limitation now stated rather than silent
A constraint character is enforced as a single split throughout — the locked-node filter screens rearrangements on it, and
impose_constraint()repairs to it and nothing else. A character with three or more states therefore has taxa nothing constrains.An intermediate version judged captures by the stricter full-Fitch reading instead. A probe on a three-state constraint then discarded all four replicates and errored with an empty pool, where a partial answer existed — the search cannot reach a standard its own repair cannot produce. The gate follows the mapping, and
.PrepareConstraint()now warns at input that an intermediate state is unconstrained.Scope
Deliberately not fixed, filed as #54: a start tree that satisfies the constraint as documented but not the stricter exact-clade form the locked-node machinery enforces still freezes the replicate. It is not a regression (identical on
99b79d8f) and returns no wrong answer — strict implies loose, so every accepted tree satisfies the documented constraint — but the replicate does no work. Closing it means picking which reading is canonical and making it true at the Wagner and sector entry points too.vignettes/search-algorithm.Rmdstates the distinction rather than papering over it.The reachability question behind #1 (whether a satisfiable constraint's violation survives all 100 Wagner reshuffles) is left open per the issue: it needs a Hamilton probe. The proven inconsistency — missing gate, missing warning — is what this fixes.
Performance
The added checks sit on the constrained path. Interleaved A/B on
Vinther2008, 20 replicates, paired seeds, againstcpp-search:An earlier revision measured 1.12 median with 11/20 seeds >10% slower. The check now short-circuits on the locked-node mapping, which is cheaper than the post-hoc Fitch check, so only an unmapped split pays for the latter.
Review
Three
external-reviewerpasses (AGENTS.md compliance, cold bug scan,git blamehistory) plus a self-review. They found: a fourthdo.callsplat site that brokeResample(constraint=, nReplicates > 1)outright with no test covering it; over-protection in the collapse rule; discarded replicates feeding the strategy bandit and coverage estimate; repair running after the deadline; and the multi-state gap above. All fixed.CI
ubuntu-arm64passes. Thewindowsjob fails atSet up R dependencies—Can't find package called MaxMin— before any code runs. This is pre-existing and branch-independent:cpp-searchitself fails identically (runs 30929069059, 30927635697), as does every open feature branch, andfeature/ci-maxmin-windowsis addressing it separately.🤖 Generated with Claude Code