Sync cpp-search from agent-issues (post-split reconciliation, pre-resolved) - #287
Merged
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>
The memo tables reserved 4096 logPVec entries. Measured at the entry high-water mark, every character '.MS_SC_THRESHOLD' admits exceeds that -- 4990, 9555, 12047, and 27951 for the gate's own worst case -- so the capacity guard fired on all of them, the exact solver returned NA, and profile parsimony fell back to Monte Carlo without the caller asking. Reserve 32768, sized to that measurement. The guard itself is untouched: it still stops the unbounded probe loop it was added for. The 2 s wall-clock budget was miscalibrated the same way. The slowest character the gate admits takes 12.7 s on a normal build, so the budget fired on legitimate work rather than on a runaway recursion; it rises to 30 s, and scales under sanitizer builds, which run one to two orders of magnitude slower and so tripped it on everything -- leaving the sanitizer inspecting the fallback path instead of the algorithm. The 5-state test's value assertions had been skipped on every machine for as long as the capacity was the binding guard, behind a message that named the time budget; they run again. The anti-hang regression no longer requires a fallback warning on an input that now completes, and an extended test covers an input that still needs one. 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>
Two high-severity (constraint-satisfiability gate accepts jointly- unsatisfiable partial-split sets; single-tip constraint splits never map in map_constraint_nodes), ten med/low gate-consistency and hygiene issues. Includes a same-day directed harness re-run confirming T-333 still holds post-T-384, and two doc-accuracy corrections in ts_constraint.h. Rebuilt on top of merged PR #44 (last_focus: 11 -> 12) and correctly placed in the newest-first round log; last_focus advanced 12 -> 13 since both areas now have a completed round on record.
`.MS_SC_THRESHOLD`'s calibration comment quoted sub-second timings for characters that in fact take 1.9 s and 12.7 s here -- ~13x optimistic, and measured on a machine and a revision that no longer exist. A comment nobody can reproduce is worse than no comment: it made the 2 s wall-clock budget look generous when it was in fact firing on legitimate work. Replace the timings with each character's peak memo-table demand, which is a property of the character rather than of the box, and record explicitly which knob is the speed/exactness dial and why it is not the clock: a budget that arbitrates makes `StepInformation()` machine-dependent, so the same data would yield different information contents on different hardware. The gate is the reproducible dial; the budget stays a backstop clear of everything it admits. No behaviour change; comments only. 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>
…tizers The previous commit raised the wall-clock budget to 30 s so that everything `.MS_SC_THRESHOLD` admits could finish exactly, on the reasoning that a clock which stops legitimate work makes results machine-dependent. That reasoning weighed reproducibility above responsiveness, and got the trade the wrong way round: a dataset is hundreds of characters, so a budget sized for the slowest one the gate admits (12.7 s here) is an hour of unresponsiveness in the bad case, for a caller who mostly wants a number back. Exactness is a refinement over an already-documented approximation; it is what should yield. So the budget returns to 2 s and is the binding limit by design, capping what a caller waits per character. Under sanitizer builds it is scaled by the instrumentation's slowdown, since a nightly memory check has no responsiveness to protect -- that scaling is the whole point of this branch, and is what stops `MaddisonSlatkin took more than 2s` from firing on every character under ASan. The cache-reserve fix stands on its own: the tables were too small for any character the gate admits, so the exact path could never complete regardless of the budget. It now completes for the characters that fit inside 2 s. Comments in all three places said the opposite and are rewritten to match, including the claim that `approx = "exact"` is a determinism escape hatch -- it waives the gate but not the budget, so it is not one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Chartered question (a bug shared by both the incremental and full-recompute Fitch/NA paths, invisible to the 2026-07-24 round's relative oracle checks) answered negative: two independent reference scorers (Sankoff DP, brute-force BGS enumeration) cross-checked against score_tree() across ~3000 datasets, 0 mismatches. Shared-correctness question closed at this tier. Eight findings filed, all perf/hygiene/unproven-invariant, none touching shared-scoring correctness: a reintroduced per-clip allocation on the NA hot path (#77), a default-mismatch between TreeLength (XPIWE) and the default TreeScorer (plain IW) that a haiku verifier initially refuted for the wrong reason and a sonnet re-check overturned (#83), plus six lower-severity gaps. Adds tests/testthat/test-ts-na-oracle.R (65 assertions, verified passing via the tarball build recipe) -- an independent-reference oracle for the BGS inapplicable criterion, which this round is also the first to pin down in writing. Corrects one stale comment in ts_fitch.cpp. last_focus: 13 -> 1, so the next round resolves to area 2.
…s sweep, 36 filed Enacts the 2026-08-04 area-12 round's proposed new focus area (statistics & support-metrics cluster headed by src/MaddisonSlatkin.cpp) at the user's explicit request, then runs its first-ever review at opus tier. 36 of 37 candidates confirmed REAL (#85-#120): 4 sev:high (a segfault via MaddisonSlatkin(steps < 0) with zero input validation; ClusteringConcordance() silently recycling a mismatched split/character index; two Consistency.R bugs corrupting the rhi statistic via a tree-blind cache key and a mis-scoped ambiguity rewrite), 12 sev:med, 20 sev:low. One high-severity candidate refuted as a stale-library artifact (the finder built against a pre-fix install; the claimed guard already exists at HEAD). 4 dead-code/typo fixes applied inline (all inside unreachable code paths or inert local bindings, no behaviour change). last_focus: 1 -> 14. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
# Conflicts: # NEWS.md
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 agent-issues#121
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#57 landed first and enacted areas 14 and 15 independently of this branch, which had enacted its own area 14 mid-round. Resolved to #57 throughout: its scope row, its `start_tier: sonnet` (a deliberate maintainer decision recorded on #42) and `N = 15`. Kept from this branch only what #57 could not know: - `R/PresentContra.R` and `R/ClusterStrings.R` added to row 14 — owned by no other row, and both reviewed by the first-ever round. - The area-14 maturity rationale no longer reads UNMEASURED: the round had already run at `opus` and returned 36 findings (4 sev:high). `start_tier` stays `sonnet` but is now inert; routing keeps the next visit at `opus`. - A one-line note on the round entry recording that #57 superseded its row, tier and `N`, and that `R/Bootstrap.R` sits in area 15 so A14-36 was filed across the boundary. The four round entries and `last_focus: 14` are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Resolving to #57 wholesale also discarded the forward-looking half of the branch's rationale, which #57 could not have contained — it was written by the highest-yield round on record. Precedence was the wrong tiebreak; these are restored on merit: - The **array-dimension-drop class** — four independent instances found in one round (`ConcordanceTable`, `ClusteringConcordance`, `Consistency`, `ClusterStrings`, all missing `drop = FALSE`). Recorded as a class to sweep, not as four findings to re-discover. - The **`R/PresentContra.R` reference-tip-mismatch angle** — read but never exercised against a forest whose trees have tips absent from the reference. - The **reachability key question** ("is this reachable from `MaximizeParsimony()`'s default output path the way #16/T-400 was, and does it return a silently wrong number rather than erroring?"), which produced three of the round's four sev:high findings. #57's questions are deeper on the numerics but do not ask this. Header no longer claims UNMEASURED: the area has a measured, heavily-yielding seam. `start_tier` still reads `sonnet` per the maintainer decision on #42, but that decision rested on "no measured yield at all", which is now false — flagged for the maintainer rather than flipped unilaterally. 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>
UBSan reported 12 nonnull-attribute violations from ts_tbr.cpp:355-357 and 370-372 -- the prelim, final_ and local_cost copies in StateSnapshot::save() and ::restore(). std::memcpy declares both pointer parameters nonnull, so passing the .data() of an empty vector is undefined behaviour even when the length is zero. The shape that gets there is HSJ data whose characters all belong to a hierarchy: the Fitch kernel is left with nothing, so total_words and n_blocks are both zero and those three arrays are empty. Confirmed by instrumenting save() and running test-ts-hsj.R, which reaches it through driven_search() from "HSJ search handles all-hierarchy data (zero Fitch words)"; the probe reported state=0 cost=0 with data() genuinely null. The NA arrays and postorder were not implicated -- consistent with the reported line set -- but are guarded on the same footing. Benign on every toolchain the package targets, and this changes no behaviour: a zero-length copy did nothing before and is skipped now. It was the only remaining sanitizer noise in an otherwise clean run, and a compiler is entitled to infer non-nullness from the attribute. Matches the guard already used in TreeState::load_tip_states(), TreeState::save_node_state() and reduce_dataset(). Fixes #124 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Anchor the expected-MI recurrence at the mode, and guard its inputs expected_mi() seeded its recurrence over the hypergeometric distribution of cell overlaps at the smallest overlap the marginals allow. That probability is around 2^-1197 for a balanced split of 1200 tips, so it underflowed to zero, and the recurrence being multiplicative, every later term stayed zero: the function returned exactly 0, and ClusteringConcordance(normalize = TRUE) silently reported uncorrected mutual information. The recurrence now starts at the mode, whose probability is the largest of at most N + 1 values summing to one and so is always representable, and walks outwards in both directions. Also: reject an ni that is not a pair, matching mi_key(); guard the log-factorial table against a negative index, which an out-of-range ni could reach; replace the GCC/Clang constructor attribute with a block-scope static, whose initialization C++17 makes thread-safe; and reject negative state codes in quartet_concordance(), which index its count buffers directly. Fixes #91 Fixes #104 Fixes #105 Fixes #107 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> @
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 agent-issues#121 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Builds with PKG_CPPFLAGS=-D_GLIBCXX_ASSERTIONS and runs the full test suite, catching container-bounds address-formation bugs (#51) that plain builds, R CMD check, Valgrind and ASan's own instrumentation all miss. Landed blocking: a build of current cpp-search under the flag plus the full testthat suite ran clean locally. Fixes #60
Sharpen the expected-MI tests and NEWS after review The NA case in the quartet block compared two structurally all-zero results, so it could not show the new non-negative guard leaves NA alone; it now uses six taxa, where dropping one would change the count. Assert the counts themselves, not just their shape. Cover the multi-state `nj` the real caller passes, and `.ExpectedMI()`, the memoised entry point through which ClusteringConcordance() reaches this arithmetic. NEWS: a wider sweep (600 partitions, N up to 3000) puts the change to already-correct values at 1.5e-11 rather than 2.3e-12, and shows the defect truncates the sum silently rather than only zeroing it -- as little as a quarter of the true value survived where some blocks underflowed and others did not. The threshold is 1080 tips scored for a character, not tips in the tree. Note the quartet guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> @
fix: loosen constraint matching to the documented free-taxa contract
Size the exact profile solver's guards, and stop them tripping under ASan
ClusterStrings(): guard the singleton-cluster colSums dimension drop (#96), cluster on the true dissimilarity matrix via as.dist() instead of raw Euclidean distance between distance-matrix rows (#97), and return a documented per-element vector plus silhouette attribute in both degenerate branches (#113). ParsSim(): reject character counts that would overflow the 32-bit Fitch bit-set representation (#118), replace an opaque sample.int() failure with a clear error when a tree cannot host the requested states (#99), and reuse the step-loop's legal-edges result instead of rescanning every character at return (#100). Fixes #96 Fixes #97 Fixes #113 Fixes #118 Fixes #99 Fixes #100 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… drop, NaN docs - .SortTokens() no longer rewrites a partial-ambiguity token as full ambiguity when the dataset's contrast holds other ambiguous tokens not present in the character being processed (#88). - ExpectedLength()'s cache key now includes a tree-derived component, so scoring different trees against the same dataset no longer collides and silently returns one tree's cached result for another (#87). - Consistency() always returns a matrix, even for a dataset that compresses to a single character pattern (#94). - Document the NaN cases in Consistency()'s @return; values are unchanged (#112). Fixes #88 Fixes #87 Fixes #94 Fixes #112 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- QuartetResolution(): handle an unresolved (star) quartet as NA instead of erroring in vapply. - WideSample(): sort the FarFirst (effort=1) selection to ascending input order, call FarFirst() with named arguments at both sites, and drop (rather than stale-copy) the firstHit attribute when subsetting a multiPhylo. - Bootstrap.R: avoid the sample() length-1 vector trap in resampling. - WhenFirstHit(): anchor the stage-name regex so a name that merely contains the pattern doesn't produce a garbled label. - TaxonInfluence(): normalize Distance()'s matrix orientation explicitly instead of assuming a fixed dimension ordering. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
review found the tee pipe swallowed R CMD INSTALL's exit code without an explicit bash shell (no pipefail), and the leg shared ubuntu's cache key, so a hardened build could silently overwrite or be overwritten by the unhardened one. Also cover tier-3 tests and fail the flag-count check on an unreadable log rather than erroring past it.
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>
…cache-aliasing # Conflicts: # NEWS.md
The previous note rested on reproducibility, which is the weaker argument: the cache already returns a stored median in preference to a fresh draw, so it does not preserve set.seed() semantics for a repeated call either way. What actually decides it is that shape-keying would buy nothing at the sizes this package handles. Comment only; no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Code review of the branch, four findings, all applied. The block comment justifying the drop of the backbone still quoted "65 of the 187 compliant topologies" on seven taxa. Those came from a probe whose compliance checker was built on as.Splits(), which omits pendant edges: its second character had a one-taxon "apart" group, which every tree separates via that group's own pendant edge, so the character read as violated in a fifth of draws and the compliant set was undercounted. NEWS, the vignette and the PR body were corrected to the enumerated 105-of-1155 figures at the time; this comment -- the one place a maintainer would look when judging whether the rewrite earned its complexity -- was missed. It now carries the right numbers and says why a pendant-blind checker undercounts, so the mistake is harder to repeat. ts_wagner.h still described the intermediate design: build the backbone, then place tips into it. The shipped code dispatches between two constructions and builds no backbone at all when any tip is free. Rewritten to state both paths and their very different costs, since that is what a caller reads to decide whether this is cheap enough to call per replicate. ts_random_constrained_tree() took tip_data straight to make_dataset(), which validates vector lengths but not tip_data VALUES; five other exports call validate_tip_data_values() at the boundary for exactly that reason. A 0 or an out-of-range index read past token_states; it now errors. rctSeparates() in the test file inherits the pendant-edge blindness described above. No current caller trips it -- every group has two taxa -- so it now checks that rather than assuming it, and says what to do instead. Verified: build clean, compile-attrs arg counts match, spelling clean, 875 assertions over 7 constraint/Wagner/driven suites pass. Both guards confirmed to fire. No change to sampler behaviour, so the measured results stand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The sampled distribution is a function of the unlabelled rooted shape: FastCharacterLength() is positional and the relabellings are uniform, so relabelling composes with a uniform permutation and leaves it uniform. Identical topologies are a subset of identical shapes, so keying on the labelled topology missed every reuse this catches and none of its own. The gain is not hypothetical. Random trees of 24+ leaves practically never share a shape, which is what an earlier measurement recorded -- but most- parsimonious trees are not random, differing only by local rearrangement. Across the 31 distinct MPTs of the 23-taxon dataset the documentation uses as its example, there are 19 distinct shapes: 39% of the work goes away. TreeTools::RootedTreeShape() enumerates shapes into an integer64 and so stops at 55 leaves, which 16 of the 30 bundled inapplicable.phyData datasets exceed. .ShapeKey() instead builds the Aho-Hopcroft-Ullman canonical code and packs it to bytes, which has no ceiling and is 12-20x shorter than the labelled key it replaces. It is verified against RootedTreeShape() as an equivalence relation where the two overlap. Sorting the child codes is what makes the encoding canonical, so it is inherently invariant to edge order and node rotation; the SortTree() and Preorder() canonicalisation this replaces is no longer needed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Break the root edge in all_tbr(), so TBRMoves() returns the complete neighbourhood
Fix eight Concordance.R bugs (red-team area 14)
Guard ts_bench_tbr_phases against zero Fitch words
Fix SPRMoves() roxygen @return copy-paste error
Widen expected-MI cache keys to the full integer range
fix: sample every constraint-permitting start topology
Fix four Consistency.R bugs: token remapping, cache collision, matrix drop, NaN docs
The July sign-off on this class was wrong twice in a week (#124, #151), both times because it argued reachability transitively rather than checking. Redo it properly, and — the part that lasts — stop relying on a human pass at all. The class needs two detectors that are blind to each other: UBSan's nonnull check sees a null `.data()` reaching `memcpy`, and hardened libstdc++ sees `&v[0]` on an empty vector, which forms an out-of-range address without ever loading from it. Both legs now exist in CI, so coverage is bounded by which *inputs* reach them — not by code reading. Static pass over all 69 memcpy/memmove/memset sites plus the .front() / .back() / .data()+i siblings: every one is guarded or provably non-empty at the site. Dynamic pass: 1704 runs under a local -D_GLIBCXX_ASSERTIONS build, over degenerate shapes x entry points x weighting modes, the 37 TS_* alternative kernels, and the >=150-tip L3b regime that no test had ever entered. No aborts — and a positive control proves that statement means something: reverting the #151 guard aborts on the first call. Adds tests/testthat/test-ts-degenerate-shapes.R (Tier 2, ~5 s), whose job is to put these shapes in front of both sanitizer legs on every dispatch. It aborts against a build with the #151 guard removed, so it is not tautological. One code fix: a 0 x 2 `startEdge` matrix passed every existing shape check and reached `flat.data() + n_edge` on an empty vector — undefined before C++20, and reported by neither detector. Fixes #177
`l3b_active` needs more than the TS_L3B_INCREMENTAL knob -- also a null sector mask, no tabu list and no pool collection -- so "the knob buys the L3b sites" was a reachability claim of exactly the kind the July audit got wrong. Checked it: TS_L3B_STATS=1 reports patch_clips=36 on the forced 12-tip call, so the path does engage. Say so where the claim is made. Also note that the zero-word datasets in the same test are the other side of that guard -- L3b is correctly inert for them -- and hedge the audit note's env-knob row, where per-knob path engagement was not verified.
Re-audit degenerate-container UB, and put the class under CI
ms609/MaxMin is renamed to ms609/Coreset. Updates the Suggests entry, the `requireNamespace()` guards, every `MaxMin::` call in WideSample(), the `MaxMin.progress` option in the Parsimony app (renamed upstream to `Coreset.progress`), the tests' `skip_if_not_installed()` and mocked-binding package, and the generated Rd. The three workflows pin prebuilt binaries by direct URL -- ms609.github.io/packages/bin/*/MaxMin_latest.* -- and GitHub Pages does not redirect, so those are repointed at Coreset_latest.*. This is why the PR is a draft: those URLs 404 until ms609/packages#2 has merged and published. Deliberately NOT renamed: - inst/REFERENCES.bib, whose Porumbel et al. title is literally "A simple and effective algorithm for the MaxMin diversity problem". A cited title is data, not an identifier. - "the MaxMin optimum" in WideSample.R, which names the objective. Coreset still solves Max-Min diversity, and still exports ExactMaxMin(). Every Coreset:: symbol used here (FarFirst, DropAdd, Grasp, ExactMaxMin) was checked against the renamed package's exports and formals. Agent work committed under ms609 because the ms609-agent account is suspended. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reconcile the two canonical cpp-search lines that diverged after the agent-issues/ms609 repo split: bring agent-issues' 73 newer commits into ms609, leaving both remotes to converge on this merge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> # Conflicts: # NEWS.md
Follow the MaxMin -> Coreset package rename
ms609
enabled auto-merge
August 18, 2026 07:42
Refresh the post-split reconciliation with agent-issues commits landed since this branch was cut (the MaxMin -> Coreset package rename, #180), so the PR is current with its merge target and CI can pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…typo The '-blather' pass shortened the two .PrepareConstraint free-taxa warnings (empty constraint / trivial constraint character), so the free-taxa test's expect_warning() patterns keyed on the old 'constrains nothing' wording no longer matched -- the warnings still fire. Repoint the four patterns at the new wording and fix the 'Igoring' -> 'Ignoring' typo in the empty-constraint message. 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
2 participants
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.
Reconcile the two
cpp-searchlines that diverged after the agent-issues / ms609 repo split.This merges agent-issues
cpp-search(73 commits) into ms609cpp-search, the samedirection as #284. It is a pre-resolved merge branch: the only conflict was an
additive
NEWS.mdoverlap (both lines appended entries), resolved by keeping both.So this PR is conflict-free and mergeable via the UI, unlike a raw cross-fork PR.
No new code: every merged commit was already reviewed and passed CI on agent-issues
cpp-search. Please merge via the UI once the required checks are green (direct pushto
cpp-searchis blocked by the "Green to merge" ruleset, by design).A small follow-up PR will then propagate ms609's own unique commits
(
feat(effort): scale the MPT-enumeration ceiling+ merges) back to agent-issues soboth lines fully converge.
Agent-prepared. Filed under ms609 because the
ms609-agentmachine account iscurrently suspended; the human's account appears as author only for that reason.