fix: loosen constraint matching to the documented free-taxa contract - #84
Conversation
`?MaximizeParsimony`'s `constraint` documents the phyDat reading: a tree is compliant when some edge separates the taxa coded `1` from those coded `0`, with `?`-coded taxa free on either side. The locked-node machinery enforced a strictly stronger one — some node's tip set had to EQUAL the `1` group (or its exact complement), free taxa excluded. Strict implies loose, so no wrong answer was ever returned. What broke was movement: a tree satisfying the documented contract without making either group an exact clade mapped to no node, which `regraft_violates_constraint()` reads as "already violating" and answers by rejecting every rearrangement. The replicate froze on its start. One reading, applied at every entry point: * `.PrepareConstraint()` now folds both groups into `consSplitMatrix` as 1 / 0 / NA, NA marking a free taxon. `build_constraint()` reads any value that is neither 1 nor 0 as free, so a hand-built 0/1 matrix (tests, `build_constraint_from_bitsets()`'s pool splits) still means "no free tips" and takes exactly the old path. * `map_constraint_nodes()` maps a split to the chain of nodes that DISPLAY it — covering one group, holding none of the other — instead of matching one exactly, and records both ends. Tips are candidates now too: a single-taxon group's "clade" is the tip itself, and scanning `postorder` alone (internal nodes only) left those splits unmapped and froze the replicate the same way. * `regraft_violates_constraint()` uses the end of that chain that permits most per question: the highest displaying node for a clip that must land inside, the tightest for one that must land outside. * `classify_clip_constraints()` reads "outside" as the apart-group rather than as `~split_tips`, so a clip of purely free taxa is UNCONSTRAINED and may be regrafted anywhere. * `wagner_tree_displays_constraint()` and `wagner_collect_active_splits()` get the same reading, so the Wagner build path cannot diverge from the search path again; `violates_constraint_posthoc()` already used it (a Fitch score against the constraint phyDat), so it is unchanged. * `impose_constraint()` no longer counts a documented-compliant tree as violating, and never moves a free taxon when it does repair one. * `ts_collapse_pool()` protects the tightest node realising each constraint rather than one matching it exactly, which with free taxa protected nothing. * A constraint character with no `0` taxa is dropped: it is vacuous under the documented contract, so enforcing its `1` group as a clade would restrict the search for nothing. `vignettes/search-algorithm.Rmd` gains a section stating the single contract. Fixes #54 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`ts_collapse_pool()` identified the branch realising a constraint by matching a node's tip set to the `1` group exactly, so with free taxa it protected nothing: the separating edge was contracted and the RETURNED tree broke the documented constraint, even though every tree the search visited satisfied it. This is the one place the strict reading did produce a wrong answer, so it gets a deterministic test of its own rather than riding on the search tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three independent reviews of the first commit. Substantive changes: * A clip whose OWN tip set displays the split is now UNCONSTRAINED (`classify_clip_constraints`). It carries the constraint with it: the node at whatever attachment point it lands on has exactly the clip's tip set, so the split stays displayed, and TBR's rerooting cannot change that. Without this, a clip containing the whole displaying chain left no surviving `below` that could be the anchor's descendant, and every regraft of a subtree that was in fact free to go anywhere was rejected. * `node_displays_split()` moves to `ts_constraint.h`, so the search's mapping, the Wagner build's check and the collapse pass's branch protection call ONE predicate instead of three lookalikes. The comment demanding they stay in step is now enforced by construction. * `wagner_tree_displays_constraint()` no longer skips the root, which made it stricter than `find_displaying_chain()` (which scans `tree.postorder`, and that includes the root): a split with an empty apart-group mapped there and nowhere else, so the two entry points disagreed about a tree every constraint is satisfied by. Including the root can never accept a violation. * The inert-character filter is symmetric. Since `build_constraint()` now swaps the two groups to canonicalise, they are interchangeable, and a test on the `1` group alone was incoherent: `c(a = 1, b = 1, c = 0)` and `c(a = 0, b = 0, c = 1)` state the same constraint and were treated differently. A group of fewer than two taxa is separated from the rest by every tree, so such a character is ignored -- with a warning, because coding only `1` and `?` almost always means "group these taxa", which is not what it says. Plus: NEWS entries for both behaviour changes; the `@param constraint` doc states the inert-character rule; `.AGENTS/memory/architecture.md` records the 1/0/NA encoding and the shared predicate; `wagner_map_complement()` loses its dead `n_tip` parameter; the TBR-only test harness moves to `helper-ts.R` instead of being copied; `wagner_tree()`'s comment about `has_posthoc` is corrected (AdditionTree DOES build the posthoc DataSet -- it reaches `ts_wagner_tree`, which bypasses the reshuffle loop, which is the real reason); and `random_constrained_tree()`'s narrower sampling is documented rather than changed, since widening it is a search-quality change to measure on its own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* The single-state constraint now warns. `.PrepareConstraint()` returned
`list()` at the `nConsStates < 2` guard before the inert-character test could
see it -- and that guard is exactly what
`MatrixToPhyDat(c(a = "1", b = "1", c = "1"))` hits, the "make these a clade"
idiom. It is the case the warning's own rationale names, and it was the one
case that stayed silent.
* The Wagner test asserted nothing. It checked that {a,b,e} ends up separated
from {c,d}, which an exact {a,b} clade satisfies too, so it passed against
the pre-fix build. It now measures the tightest node covering {a,b} and
avoiding {c,d}: pre-fix that node is EXACTLY {a,b} in 25 of 25 seeds -- every
`?` taxon forced out of the constrained clade -- and now holds a free taxon
in all 25.
* The over-loosening guard needed a guard: its start already has {a,b} as a
clade, so a frozen search would have satisfied it for the wrong reason. It
now asserts the score improved as well.
* `random_constrained_tree()`'s new comment claimed its exact-clade sampling is
"always legal". True only when the together-groups are laminar, which
`.PrepareConstraint`'s four-gamete gate does not guarantee; the non-laminar
case is handled by the T-329 collapse path and the post-hoc check, not by
the claim. Corrected.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#59 landed on the trunk meanwhile, enforcing the constraint at the start tree, the pool capture and the collapse. It reads the contract the same way this branch does, so the two agree on intent -- but it routed the apart-group to C++ as a second matrix (`consZero`) beside `consSplitMatrix`, where this branch folds both groups into `consSplitMatrix` as 1/0/NA. Two channels for one fact is the shape of the bug this branch is fixing, so the merge keeps one: the folded matrix, which every kernel already takes. - `ts_collapse_pool()` derives both groups from the folded matrix and drops the now-dead `consZero` argument (9 args; init.c updated to match). Its protection rule is #59's, which is the better one: protect a realising node only when no other realising node already survives the contraction, rather than pinning the tightest one unconditionally. Both blocks #59 and this branch had -- exact-match and loose -- collapse to the single loose test, now spelled with the shared `ts::node_displays_split()`. - `.ConstraintViolated()` is handed the folded matrix, so it reads "1" rather than truthiness; NA_INTEGER is truthy and would otherwise have propagated through the accumulation and made every comparison NA. - `constraint_satisfied()` no longer claims the locked-node mapping is the stronger test. Since this branch it asks exactly the documented question, which is why the Fitch fallback now fires only on a genuinely unmapped split. - NEWS and the vignette carry both changes; the vignette's "the filter reads the constraint more strictly" paragraph goes, having become false. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Trunk moved again mid-resolution (#75). Clean merge; the only overlap is ts_constraint.h, where the incoming comments document regraft_violates_constraint() as screening-only and impose_constraint() as a heuristic every caller must re-verify. Both hold unchanged under the loosened reading -- the mapping they name is now the documented test rather than a stricter one, so "re-verify that every split mapped" means exactly "re-verify the contract". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Conflicts resolved; two decisions worth your eyeRebuilt against the trunk twice — The textual conflicts were Two channels for one fact is the shape of the bug this PR fixes, so I kept one. 1. Dropped 2. Took #59's collapse rule over mine. Mine pinned the tightest realising Also: The vignette keeps #59's "Starting trees under a constraint" section and drops Verification: full suite 11971 pass / 0 fail (4 warnings, all pre-existing Unchanged and still flagged from before: the inert-character filter drops a |
Three documentation conflicts, no code ones -- #84 and #59 both landed on cpp-search while this branch was open. NEWS.md and .AGENTS/memory/architecture.md: this branch carried its own copy of #84's constraint entry, written before #59's merge reconciled the two. Dropped in favour of the reconciled version now on cpp-search -- which is also the correct one on the inert-character rule, where this branch's copy still said "no `0` taxa" rather than "either group under two taxa". Only the random-start entry is this branch's own, and it is appended after them. vignettes/search-algorithm.Rmd: both sides added prose after the same paragraph. #59's is about repairing a violating start, this branch's about which legal starts can be drawn; they are complementary, so the new "Starting trees under a constraint" heading now covers both. Verified after resolving: build clean, compile-attrs clean, 15 constraint / Wagner / driven / sector / pool / fuse / strategy suites pass (1101 assertions, 0 failures, 0 warnings), spell check clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fixes #54
The maintainer picked "loosen the machinery to match the documented contract",
and that is what this does.
?MaximizeParsimony's@param constraintpromisesthe phyDat reading — a returned tree is compliant when some edge separates the
taxa coded
1from those coded0, with?-coded taxa free on either side.The locked-node machinery enforced a strictly stronger one: some node's tip set
had to equal the
1group (or its exact complement), free taxa excluded.vignettes/tree-search.Rmdalready told users "The position of taxa not listedin a constraint will not be constrained." That sentence was false before this
change — an unlisted taxon was pinned to the far side of the constraint — and is
true after it.
One reading, at every entry point
.PrepareConstraint()folds both groups intoconsSplitMatrixas1/0/NA,NAmarking a free taxon;build_constraint()reads any value that isneither
1nor0as free. A hand-built 0/1 matrix therefore still means "nofree tips" and takes exactly the old path — which is what the existing tests and
build_constraint_from_bitsets()'s consensus splits rely on. No Rcpp signaturechanged, so
RcppExportsandTreeSearch-init.care untouched.ts::node_displays_split()(ts_constraint.h) is now the definition of"this node displays the split", called by every implementation that used to
answer it separately:
map_constraint_nodes()(search/TBR)wagner_tree_displays_constraint()(Wagner build)ts_collapse_pool()(output collapse)violates_constraint_posthoc()(retry/revert)classify_clip_constraints()reads "outside" as the apart-group rather than as~split_tips, so a clip of purely free taxa isUNCONSTRAINED; and a clip whoseown tip set displays the split is
UNCONSTRAINEDtoo, because it carries theconstraint with it — the node at whatever attachment point it lands on has
exactly the clip's tip set, and TBR's rerooting cannot change that.
regraft_violates_constraint()uses the end of the displaying chain that permitsmost per question: the highest node for a clip that must land inside, the
tightest for one that must land outside.
impose_constraint()no longer counts adocumented-compliant tree as violating, and never moves a free taxon when it does
repair one.
map_constraint_nodes()also scans tips now.tree.postorderholds onlyinternal nodes, so a single-taxon group — whose "clade" is the tip itself —
mapped to nothing and froze the replicate the same way.
The issue understated one thing
The issue says "no wrong answer is ever returned". That holds for the search but
not for the returned result.
ts_collapse_pool()identified the branch realisinga constraint by exact match too, so with free taxa it protected nothing and the
separating edge was contracted away. Deterministically, on
(((a,e),(b,f)),(c,d))withcons = c(a=1, b=1, c=0, d=0, e:f="?"):Independently reproduced by review: 3 of 30 seeds on a 12-taxon panel returned a
violating tree before, 0 after. It now has its own regression test.
Behaviour change that needs your eye
A constraint character whose
1or0group holds fewer than two taxa is nowignored, with a warning. Every tree separates such a group from the rest, so it
constrains nothing under the documented contract — but this is wider than "no
0taxa": it also drops a one-taxon
0group, which is how some users write "makethese taxa a clade" (
c(a=1, b=1, c=1, d=0)with the rest left?).Under the loose reading that constraint no longer guarantees
{a,b,c}monophylywhether or not the row is kept — that follows from the fix you chose, not from
the filter. Keeping it would only add a spurious search restriction with no
guarantee attached, so it is dropped and the user is told. Both vignette examples
in
tree-search.Rmdcode every taxon explicitly and are unaffected.The test is symmetric in the two groups, which the old one was not: since
build_constraint()swaps the groups to canonicalise, they are interchangeable,yet
c(a=1, b=1, c=0)andc(a=0, b=0, c=1)used to be treated differently.NEWS carries both halves. If you would rather this stayed enforced, say so and
I will keep the row and restrict only the spurious part.
Deliberately not done
random_constrained_tree()still nests the together-group as an exact clade,so a free taxon never starts inside the constrained group. Every tree it
builds is compliant, so this is a sampling limitation, not a correctness one;
widening it changes which start trees the search sees, which is a
search-quality change to measure on its own. Documented in place.
constraint_node_hianchor (the "must land inside" end of the chain) issound and is the correct reading, but its search benefit is unmeasured —
on a 30-seed panel it was within noise of using the tight anchor alone, and no
test discriminates it, because TBR can usually reach the same topology by
clipping a free taxon instead. Its cost is one parent walk per split per remap,
bounded well below the postorder scan it accompanies.
Verification
failures, 0 warnings.
tests/testthat/test-ts-constraint-free-taxa.R: the freeze (TBR-only, sono other phase can mask it), end-to-end compliance, the collapse regression,
the Wagner build, the
.PrepareConstraintencoding, and an over-looseningguard that a pure 0/1 constraint is still enforced exactly. 12 of its
assertions fail against a pre-fix build of the same tree. The Wagner
assertion is the sharpest: pre-fix the tightest node covering
{a,b}andavoiding
{c,d}is exactly{a,b}in 25 of 25 seeds — every?taxonforced out — and holds a free taxon in all 25 after.
every finding is addressed in the follow-up commits or answered above.
🤖 Generated with Claude Code