fix: sample every constraint-permitting start topology - #122
Open
ms609 wants to merge 3 commits into
Open
Conversation
random_constrained_tree() built its backbone from the "together" group alone,
so every tip the constraint does not name became a root-level item: placed
outside every constrained group, with each group an exact clade. Strict
implies loose, so every tree it returned was compliant — but most compliant
trees were unreachable at every seed.
Exhaustively, on 6 taxa with one constraint character ({c,d} together, {a,b}
apart, {e,f} coded `?`), 35 of the 105 unrooted binary trees comply and the
generator could draw only 15 of them.
The backbone is now built from the named tips, and each free tip is inserted at
a uniformly random edge of it — inside a constrained group as readily as
outside one. A free tip is in neither group of any split, so wherever it lands
the edge that separated the groups still separates them. With no free tips the
two versions are the same function down to the RNG draw sequence, which keeps
build_constraint_from_bitsets()'s consensus constraints on their old behaviour.
ts_random_constrained_tree() exposes the generator so tests can see the tree
the search STARTS from; going through MaximizeParsimony() cannot, because TBR
rearranges whatever it is handed.
Fixes #121
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
|
🔴 The residual must be fixed before we merge |
Author
|
Heads-up: this PR went I have deliberately not fixed it from here: Once #84 merges, the tidy unwind is to retarget this PR's base to |
The nesting-of-groups backbone can build only trees in which each "together" group is an exact clade and the clades sit as siblings. With free taxa that is a small corner of what the contract permits: a clade may take on any taxon its character does not name, and two characters with disjoint groups may nest either way round. Exhaustively, of the trees a constraint permits it could draw 15 of 35 (six taxa, one character) and 105 of 1155 (eight taxa, two characters); the free-taxon scatter of 3fa12e9 lifted the first to 35 but left the second at 105. Constraints with no free taxa keep the backbone: the groups then pin every clade, so it is already complete and uniform, and the consensus caller (build_constraint_from_bitsets) is untouched, draw for draw. Otherwise the tree is now grown a tip at a time. Named tips first: a rejection pass takes an unconstrained random tree and keeps the first compliant one, which is exactly uniform, and where the constraint is too tight for that to land, a legality-filtered pass always does. The filter is regraft_violates_constraint() with the new tip as a one-node clip, over constraint masks restricted to the tips placed so far. A partial tree that displays every restricted split can always be extended, so filtering never paints the construction into a corner, and every compliant tree is built by whichever insertion order matches it. Unnamed tips go in last, unfiltered, so their placement stays exactly uniform. Measured: both cases now reach every compliant tree (35/35, 1155/1155), with 0 violations in 20000 draws each and per-tree counts consistent with uniform. Fixes #121 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
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
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 #121
Follow-up to #84, which loosened constraint enforcement to the documented
free-taxa contract. This branch started stacked on it; now that #84 has merged
the diff here is only the start-tree generator.
random_constrained_tree()— theRANDOM_TREEstart strategy, reached fromts_driven.cpp:124— built the tree from a backbone: one clade per constraintcharacter, nested as their "together" groups are nested. Every tree it returned
was compliant, but that construction can only ever produce trees in which each
group is an exact clade and the clades sit as siblings. With free taxa
the contract permits far more: a clade may take on any taxon its character does
not name, and two characters with disjoint groups may nest either way round.
What was unreachable
Small enough to enumerate exhaustively. Trees drawn over 20 000 seeds:
99ca3f04)3fa12e92)Not "rare" — unreachable at every seed. The middle row is the first commit on
this branch, which fixed the taxa a user codes
?but not the taxa named byanother character; that is the residual this replaces.
The change
Constraints that name every taxon keep the backbone. With no free taxa the
groups pin every clade exactly, so the backbone is already complete and uniform
— and this is the path
build_constraint_from_bitsets()'s consensus splitstake, so the cross-replicate consensus-tightening machinery is untouched, draw
for draw.
Otherwise the tree is grown one tip at a time. Named taxa first, in two
passes:
first compliant one. Uniform over compliant trees exactly. Lands whenever a
workable fraction of trees comply, which is the loose or small constraints a
user typically writes; bounded at 64 tries.
those that leave every constraint still displayed. Always lands.
Then the taxa no character names go in, unfiltered — so their placement is
exactly uniform whichever pass built the rest. A constraint on a handful of taxa
in a large matrix is therefore sampled uniformly end to end.
Completeness rests on two facts, both stated in the code comment: a partial tree
that displays every constraint restricted to the taxa placed so far can always
be extended, so filtering on that condition never paints the construction into a
corner; and the filter rules out only edges that break it, so every compliant
tree is built by whichever insertion order matches it.
The filter itself is
regraft_violates_constraint()— the existing per-candidateTBR test — with the arriving tip as a one-node clip, over constraint masks
restricted to the placed taxa. Without the restriction no node covers a group
whose taxa have not all arrived, and every edge looks illegal.
Uniformity
Measured against the exhaustive sets above:
114.3, χ² p = 0.52.
of 17.3 — consistent with uniform (σ ≈ 4.2). Before the rejection pass was
added the same measurement gave 1–100, which is what filtered insertion alone
looks like.
Where the rejection pass cannot land — a constraint pinning down most of a large
tree — the fallback still reaches every legal topology but favours some over
others. That limit is stated in the code, the vignette and NEWS rather than
glossed.
Verification
tests/testthat/test-ts-random-constrained-free.R: the exhaustive6-taxon soundness + completeness + uniformity triple; a new 8-taxon
two-character test asserting that both nesting orders and the sibling
arrangement all occur and that 4000 draws reach over 1050 of the 1155
compliant trees (both earlier versions top out at 105); the
?-taxon caserouted through
.PrepareConstraint(); an over-loosening guard that aconstraint naming every taxon still builds exact clades — which also pins the
dispatch; and a node-budget check. The new test fails against a build of the
previous commit.
1098 assertions, 0 failures, 0 warnings.
varying free fractions: 0 contract violations, 0 malformed trees. 81% of
splits now hold a free taxon inside the group, against 53% for the scatter and
18% before.
enough that rejection never lands, so the filtered pass is what runs. 0
violations in 200 draws at 6.2 ms per draw, with the constrained clade ranging
over 23–37 taxa rather than the 20 the backbone would always give.
ts_random_constrained_tree()is a new internal Rcpp export (registered inTreeSearch-init.c, not inNAMESPACE), added by the first commit on thisbranch. The generator is otherwise unobservable: through
MaximizeParsimony()you see what TBR made of the start tree, not what was drawn.
🤖 Generated with Claude Code