C++ search - #210
Draft
ms609 wants to merge 1265 commits into
Draft
Conversation
ms609
marked this pull request as draft
March 25, 2026 14:21
ms609
added a commit
that referenced
this pull request
Mar 28, 2026
ms609
added a commit
that referenced
this pull request
Mar 28, 2026
ms609
added a commit
that referenced
this pull request
May 18, 2026
In R CMD check, R runs as a non-interactive subprocess with captured stdout. R_FlushConsole() calls fflush() on that pipe; when the buffer fills the call blocks indefinitely, causing the 6 h GHA timeout seen on every ubuntu runner for PR #210. Gate the \r-overwrite progress line and the flush behind R_Interactive (FALSE in batch/check contexts). Interactive sessions are unchanged. At verbosity >= 2 in batch mode, emit plain \n-terminated lines so diagnostic logs still carry progress detail without the flush risk. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ms609
added a commit
that referenced
this pull request
May 18, 2026
In R CMD check, R runs as a non-interactive subprocess with captured stdout. R_FlushConsole() calls fflush() on that pipe; when the buffer fills the call blocks indefinitely, causing the 6 h GHA timeout seen on every ubuntu runner for PR #210. Gate the \r-overwrite progress line and the flush behind R_Interactive (FALSE in batch/check contexts). Interactive sessions are unchanged. At verbosity >= 2 in batch mode, emit plain \n-terminated lines so diagnostic logs still carry progress detail without the flush risk. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced May 19, 2026
ms609
added a commit
that referenced
this pull request
Jul 3, 2026
…lement `AdditionTree(constraint = )` silently returned constraint-violating trees for about one addition order in eleven. A constraint split is an *unrooted* bipartition, but `wagner_collect_active_splits()` could only express "inside" as a rooted subtree, so when the added inside tips straddled the construction root their LCA was the root and the split was skipped. An LCA never moves back down, so the skip latched: one unlucky base triple left the constraint unenforced for every remaining insertion, and no warning was emitted because `constraint_fallback` never fired. Enforce through the complement instead. At most one side of a split can straddle the root, and making the outside set monophyletic displays the same bipartition, so `wagner_map_constraint_nodes()` now also maps the complement -- but only for the splits that need it, and `use_complement` latches so a mapped split costs one postorder scan per step, not two (the latch is sound for the same monotonicity reason the bug was permanent). If both sides straddle, the partial tree already fails to display the split and no insertion can repair it; that case is left to the post-hoc check. T-364 and T-370 are the same defect filed twice, and each had an unmerged fix (`bdc32fb2`, `355c4196`) written against the pre-T-368 layout, which still contained the per-tip scan that HEAD replaced with the `n_needed == 1` shortcut. This ports their shared technique onto the current layout rather than reverting that, and takes `bdc32fb2`'s tests. Tests are sweeps, not single calls, because the failure was order-dependent. On the code before this commit they report 35/400 seeds, 16/120 base triples (a two-taxon group) and 42/120 base triples (a three-taxon group); after it, zero of each. The pinned `set.seed(1)` in the existing test is no longer load-bearing -- which matters, because that pin passed on Windows/x86 and failed on the ARM64 CI runner, red-lighting `sense-check` and skipping the coverage wave with it. KNOWN COST, constrained *search* only (`AdditionTree()` itself is unaffected -- it has `has_posthoc = FALSE`, so it never retried and never paid for retries). On a 22-tip probe the constrained search is ~4.7x slower at an identical score and identical constraint compliance. The cause is not this fix: 19 of 200 start trees now display the constraint with the *complement* as the rooted clade, and `map_constraint_nodes()` demands an exact rooted-clade match, so it returns -1 and `regraft_violates_constraint()` (ts_constraint.cpp:355) then rejects *every* move -- the same rooting-blindness this commit fixes in Wagner, still present downstream. `reroot_at_tip0()` in ts_fuse.cpp already establishes exactly the invariant that would fix it. Filed rather than fixed here: it touches a shared header and the fuse path, and wants its own measurement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…can find it Completes the previous commit. Enforcing a split through its complement is correct but can leave the complement as the rooted clade, and the rest of the constraint machinery is not rooting-agnostic: map_constraint_nodes() looks for a node whose subtree mask *equals* the split (canonicalised by build_constraint() with tip 0 outside), and when that search fails regraft_violates_constraint() reads the -1 as "tree already violates" and rejects every move (ts_constraint.cpp:355, whose own comment says this "shouldn't happen if the starting tree is valid" -- it now could, because the tree was valid and merely rooted elsewhere). Measured on a 22-tip constrained search: 19 of 200 start trees displayed the constraint with the complement as the rooted clade -- 9.5%, matching the rate at which the original bug fired -- and the search took 4.7x longer to reach an identical score with identical constraint compliance (0.33s vs 0.07s median, three interleaved rounds, reproducible to 2 dp). Every phase was slower on a score-identical trajectory, which is the signature of moves being rejected rather than of a different search. Rooting on tip 0 makes the non-tip-0 side of every displayed split a clade, so the mapping always succeeds when the tree really does display the split. reroot_at_tip0() already existed in ts_fuse.cpp for the same reason (fuse compares differently-rooted trees); it is now declared in ts_fuse.h rather than duplicated. After it: 200/200 rooted-clade, and the wall gap is gone -- 0.07s vs 0.07s over three interleaved rounds. So there was no correctness-versus-speed trade-off here to weigh: the cost was an artefact of handing a downstream check a rooting it could not read. This does NOT resolve that rooting-blindness in general -- map_constraint_nodes() is still exact-match, so any other producer of a validly-but-differently-rooted tree will trip the same branch. test-ts-wagner.R:394 asserted sisterhood of tips 1 and 2, which held only in the rooting the constrained build used to return; it now tests for the unrooted split it meant all along. Verified with a from-clean rebuild (CCACHE_DISABLE=1, force), since ts_fuse.h gained a declaration and R's build does not track header deps. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…pping) T-364 is marked FIXED against `7685bf07` + `796a29d3`, with the note that T-370 is the same defect and that the two branch fixes are superseded rather than pending. Without that the next finder re-hunts a closed bug, and the next maintainer merges a stale fix. T-384 is the defect found while fixing it, and is the more interesting one: `map_constraint_nodes()` tests whether the canonicalised split is a *rooted clade*, but a split is an unrooted bipartition displayed whenever either side is a clade -- so a valid tree rooted the other way maps to -1, and `regraft_violates_constraint()` answers that by rejecting every move. Filed P2, not P1: nothing wrong is returned, the search just cannot move. The row records the measurement (19/200 start trees, 4.7x wall at an identical score), the diagnostic signature that identified it (every phase slower on a score-identical trajectory = rejected moves, not a different search), the three fix options with the reason (a) is not free, and the honest scope of the current workaround: re-rooting fixes the one producer, while fuse/sector/parallel still call the mapping on trees they did not root. It also flags that T-370's "~1.43x wall to match the old score" -- the figure that kept that fix on a branch -- was measured in exactly the configuration this finding describes, and that the gap vanished here once the tree was re-rooted. Not shown to be the same artefact; flagged for re-measurement rather than retracted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ance
Add `unit = c("quartet", "trit")` to QuartetConcordance(). "quartet"
(default) is unchanged; "trit" scores in Nelson & Ladiges (1992)
redundancy-corrected currency, where a split of sizes (k, t-k) carries
only (k-1)(t-k-1) independent quartets (4-cycles of K_{k,t-k}; the N&L
entailment is GF(2) cycle addition).
Scoring uses coverage ("option b"): per state-pair, concordant trits
A over the reported unit's own content (Wk for edges, Wc for chars),
pooled by shared information M = min(Wc, Wk). Only a split *displayed*
by the character scores 1; nested/crossing get strict partial credit.
Multistate is handled in the same currency, not deferred: a quartet is
decisive only within a state-pair, those K_{n_i,n_j} blocks are
edge-disjoint, and the entailment never crosses them, so trits add over
pairs (W = sum (n_i-1)(n_j-1), weight 4/(n_i n_j)). Verified by GF(2)
rank of the decisive/concordant sets on 2/3/4-state characters. Missing
/ ambiguous / inapplicable tokens drop per character by construction.
Validation (dev/benchmarks/frac-quart/): cell extraction cross-checked
against the C++ kernel; A <= min(Wc, Wk) everywhere; spec ladder
reproduced; package == an independent re-implementation on binary,
multistate and missing-data inputs across all weight x return combos.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a random-expectation baseline to QuartetConcordance(unit = "trit"), mirroring ClusteringConcordance()'s normalize interface: normalize = FALSE (default) no correction; published measure unchanged normalize = TRUE exact expected concordance under a fixed-marginal null normalize = <int> Monte-Carlo tip-shuffle estimate of that expectation Null model: reassign each character's tokens across the leaves at random, holding state counts and split sizes fixed (the multivariate-hypergeometric confusion table; the same null as ClusteringConcordance's miRand and Consistency's ExpectedLength). This is the only coherent null for the trit currency -- it re-zeros each (split, char, state-pair) against its own combinatorial floor and extends unchanged to multistate + missing data. The flat 1/3 SCF baseline is a raw-quartet heuristic that does not survive the trit floors; it is documented as the rejected alternative. Implementation: only A (and, for multistate pairs where a pair's side-A count is random, wk and M = min(wc,wk)) vary under the null, so the exact estimator accumulates E[m], E[m*A/wk], E[m*A/wc] from the hypergeometric pmf (cached on (n_i,n_j,M,t)) and re-zeros the pooled edge/char ratio against the pooled expectation, with the same weight aggregation as the observed score. .Rezero guards the z->1 blow-up (-> NA) and returns values unclamped; .Rezero(1,z) = 1 so a displayed split still scores exactly 1 (ceiling invariant preserved), while conflicting characters go negative. Chance correction for the raw unit="quartet" currency is deferred (it needs the per-state-pair cells the C++ kernel does not expose, and a decision on its null) -- normalize errors there for now. Validated: exact vs MC tip-shuffle agree within MC error at the cell level (dev/benchmarks/frac-quart/chance_baseline.R) and end-to-end (test-Concordance.R). Design settled in dev/plans/frac-quart-weight-agreement.md section 7. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…istate
Two gaps in the unit="trit" chance correction, both in the multistate path:
- weight=FALSE re-zero averaged observed and baseline over DIFFERENT
(split, char) cells: observed drops a cell when denM==0 (na.rm), but the
baseline kept it whenever baseDenM>0 (a degenerate multistate pair can have
observed wk=0 yet E[m]>0). Mask both to NA on the union of NA cells so the
edge/char row-means cover the same population before .Rezero. weight=TRUE
(ratio-of-sums) is unaffected.
- The exact-vs-MC end-to-end test used only binary characters, so the
random-wk multistate regime -- the reason E[A] was elaborated into
E[m*A/wk] + E[m] -- had no oracle. Add a 3-state column and cover
edge/char x weight in {TRUE, FALSE}, asserting exact and MC agree on which
cells are NA (guards the cell-matching fix) and within MC error elsewhere.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tet" Extend the chance-correction option to the raw quartet currency, using the same fixed-marginal (hypergeometric) null as the trit path. Per state-pair the raw concordant/decisive counts are polynomials in the 2x2 cells (no floors), so the exact expectation E[conc], E[dec] is a clean sum over the same trivariate-hypergeometric pmf (.ExpectedQuartet / .QuartetExpect); MC (.QuartetMC) reshuffles tokens and re-scores through the C++ kernel. The pooled conc/dec ratio is re-zeroed against E[conc]/E[dec] (weighted) or with the same NA-cell matching as the trit weight=FALSE path (unweighted). Removes the guard that errored on unit="quartet", normalize != FALSE. .Rezero(1, z) = 1, so a displayed split still scores exactly 1; conflicting characters go negative. normalize = FALSE is byte-identical to the published raw measure. Validated exact vs MC tip-shuffle across return x weight x binary/multistate (test-Concordance.R); all Concordance tests green. Design note in dev/plans/frac-quart-weight-agreement.md section 7 updated (raw unit DONE). This is the R reference for the forthcoming C++ port of the same expectation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Move the exact hypergeometric expectation for QuartetConcordance(normalize = TRUE) from R to C++ (src/concordance_expect.cpp): `quartet_expect` returns E[conc], E[dec]; `trit_expect` returns E[m], E[m*A/wk], E[m*A/wc]. Both sum the per-state-pair trivariate-hypergeometric double sum, hoisting the log-choose terms out of the M/p/r loops (the dominant cost). The R exact helpers (.ExpectedTrit/.CharTritExpect/.ExpectedQuartet and the per-char .QuartetExpect loop) are replaced by thin wrappers / a single all-characters call in .TritConcordance; the Monte-Carlo (normalize = <int>) path stays in R. New Rcpp exports are registered in the manual R_CallMethodDef table (src/TreeSearch-init.c; the package uses R_useDynamicSymbols(FALSE)). Speedup at split-support scale (48 tips, 96 chars): exact normalize=TRUE quartet 2.88s -> 0.14s (~20x), trit 3.48s -> 0.45s (~8x); a full 1000-matrix run drops from ~100 min to ~10 min. Validated bit-for-bit (max |C++ - R| ~1e-14) against a self-contained R reference across binary/multistate/missing data (dev/benchmarks/frac-quart/cpp_expect_parity.R); the exact-vs-MC end-to-end tests in test-Concordance.R now exercise the C++ path and pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
All six search-kernel entry points (nni_search, spr_search, tbr_search, ratchet_search, drift_search, stochastic_tbr_phase) bailed out whenever `ds.total_words == 0`, on the assumption that no Fitch words means every topology scores the same. That's true for EW/IW/profile but false for HSJ/XFORM: .NonHierarchyWeights() zero-weights hierarchy-only patterns, build_dataset() drops weight-0 patterns, and the resulting total_words == 0 dataset's hierarchy DP / Sankoff term is still fully topology-dependent -- so the start tree was silently returned unsearched. Reachable via an all-hierarchy matrix or, more commonly, an ordinary hierarchical bootstrap replicate that (~1% of draws, by the binomial math) samples zero copies of every free-character unit. Add DataSet::topology_independent() (total_words == 0 AND hierarchy_blocks empty AND sankoff_n_chars == 0) and use it at all six bail sites in place of the bare total_words == 0 check. For EW/IW/profile this predicate is identical to the old check (hierarchy_blocks/sankoff_n_chars are only populated under HSJ/XFORM), so behaviour there is unchanged. Removing the bail exposed a second, distinct hazard: several kernels' candidate-scan machinery unconditionally constructs pointers into total_words-sized buffers (tree.prelim, edge_set_buf, from_above, ...) via `&buf[idx * total_words]`, which is UB on an empty vector regardless of whether the result is dereferenced (confirmed empirically: aborts under -D_GLIBCXX_ASSERTIONS on a full rebuild). tbr_search/spr_search/drift_phase/ compute_insertion_edge_sets/patch_insertion_edge_sets/try_root_edge_moves are patched to either null-guard the pointer (harmless: every consumer loops over ds.n_blocks, == 0 here, so the value is never read) or skip a Fitch-only sub-phase entirely when total_words == 0 -- tbr_search's exact accept-time full_rescore() remains the source of truth throughout, so search correctness is preserved even where candidate screening degrades (the same "screening degraded, accept exact" shape as T-377). stochastic_tbr_phase (annealing) has no HSJ/XFORM-aware fallback at all and stays a guarded no-op for that case; ts_driven_search now warns once, on the R thread before any worker spawns, when annealCycles > 0 would otherwise silently do nothing. Verified against dev/red-team/heavy-tests/hsj-totalwords-zero-noop.R (fails pre-fix, passes post-fix, both in a normal build and a full clean -D_GLIBCXX_ASSERTIONS rebuild) and the full existing test suite (one pre-existing, documented, unrelated flake in test-AdditionTree.R / T-364). Adds an R-level regression test exercising the same ts_driven_search path Resample()'s hierarchical-bootstrap code calls. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Update the T-373 row with the fix commit, what changed, and residual scope (stochastic_tbr_phase stays a guarded no-op for HSJ/XFORM, now warned at the R entry point). Row kept per convention -- tidy pass archives it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Correct "all six bail sites" to five (ts_temper.cpp keeps the bare total_words == 0 check by design -- it has no HSJ/XFORM fallback at all). - Confirmed the new R-level regression test genuinely fails against a pre-fix build (not vacuous) before confirming it passes post-fix. - Replace "full suite passes" with what was actually run: test_dir on the installed tests, plus the three TREESEARCH_EXTENDED_TESTS-gated files that touch the changed kernels (ts-ratchet-stress, ts-resample-stress, ts-tbr-bench), all green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… to gate it `exact_verify_sweep` is 97.7% of `tbr_search` wall on inapplicable data (dev/profiling/na-exact-verify-dominates.md), and nothing in `tbr_search` knew whether its caller needed a certified optimum. `certify_unrooted` says so; internal sub-searches whose tree is never reported (ratchet, drift, sector, fuse, nni-perturb, prune-reinsert, the anneal reconverge) clear it. The gate is OPT-IN via TS_NA_NOCERTIFY, so this commit is byte-identical to its parent: with the variable unset every caller certifies as before. The default flips only if the floor-attainment panel earns it -- skipping certification returns worse trees, so this is a quality-for-speed trade, not a free win. Two things the measurement note could not have known, both from the new counters: * `do_reroot` requires `tabu_size == 0`, and the shipped presets set tabuSize = 100 (default) / 200 (thorough). The 97.7% was measured through ts_tbr_search / ts_ratchet_search, which leave it at 0. Under the default preset the certifier is reached ONLY by the sector sub-searches (and the fuse cleanup), which leave tabu_size at 0 -- never by the ratchet, drift, or the driven polishes. So the tree a replicate reports has never been certified, while trees nobody reports are certified repeatedly. * Certification is not merely a proof: on Zanol2014 (74t) under the default preset it is worth 5 steps (1321 certified vs 1326 not), because under Brazeau's three-pass it is the only EXACT step, so it finds NA improvers the approximate scan cannot see. Gating it is a real quality loss to be measured, not a formality. Hence TS_NA_FINAL_CERTIFY (also default off): one certification of the tree each replicate actually contributes, which is where the spend is worth something. That is the third arm of the panel, and the answer to "should a cleared flag still certify a new global best" -- certify the reported result once, rather than threading the caller's incumbent through TBRParams. `na_n_evs` / `na_n_evs_skipped` are counted unconditionally and surfaced on the production entry point as `attr(, "naDiag")`. Without the skipped count, "the flag changed nothing" is indistinguishable from "the flag never fired" -- and given the tabu_size finding, that is the likelier reading of a null result. On the skip path `best_score` is re-synced explicitly: `exact_verify_sweep` did that on both its exits, and a score that drifts from its returned topology (ts_tbr.cpp:660 bug class) would read as a quality regression in the panel when it is really a reporting bug. Pinned by test-ts-na-certify.R. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five arms per cell, all on one node: A_certify (today) / B_gate / C_final at a
fixed replicate budget, then B_gate_mw / C_final_mw at arm A's OWN measured wall.
The matched-wall arms are not optional -- a wall win is spendable on replicates,
and a fixed-replicate comparison cannot see that. Local pre-read: Griswold1999
seed 1 goes 407 -> 408 at fixed replicates and back to 407 at matched wall, on
64 replicates instead of 4.
Panel is all 30 bundled inapplicable matrices, native, 5 seeds, x tabuSize
{100, 0} = 300 tasks. tabuSize is an arm dimension because do_reroot gates the
certifier on tabu_size == 0 and the shipped presets set 100/200: at 100 only the
sector sub-searches reach it (4 sweeps on Vinther2008), at 0 every whole-tree
search does (40). Thirty matrices rather than the profile's four because a sign
test at n = 4 bottoms out at p = 0.125.
The analyser enforces the rules that have previously been got wrong here: floor
attainment not any improvements-per-second rate; one number per MATRIX before any
paired test; wall as median + sign count + count >10% slower, never the mean; and
n_evs_skipped printed per arm so a null result cannot be read as "the lever is
worthless" when it means "the flag never fired".
na_n_evs_improved is now counted unconditionally rather than only under
TS_NA_TIMING. It is how often certification found a real improver rather than
merely proving optimality -- the mechanistic explanation of any reach the gate
costs. Without it a panel can report that reach dropped but not why.
dev/profiling/na-certify-gate.md records two corrections to the premise of
na-exact-verify-dominates.md: the 97.7% was measured at tabu_size == 0, which is
not the shipped recipe; and certification finds improvers rather than only
certifying, so it is worth 5 steps on Zanol2014.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…udget The TS_NA_FINAL_CERTIFY pass runs after the outer-cycle loop's own timeout checks, and tbr_search polls check_timeout only every n_tip clips. A replicate reaching it with the clock already expired would still pay a whole O(n^3) certification -- seconds to minutes on an 88-tip matrix. That breaks the maxSeconds contract for the user, and in the matched-wall A/B it silently hands that arm more wall than the arm it is compared against. Skip outright when the budget is spent. The analyser now audits this rather than trusting it: per-matrix wall / mwBudget for every matched-wall arm, with a warning above 1.25x. An arm handed more wall than arm A is not comparable to it, and the overshoot grows with matrix size because certification is O(n^3) -- so it grows exactly where the lever matters most. The panel array was built before this guard, so its C_final_mw rows are the unguarded behaviour; the audit is how that gets caught instead of assumed away. Also records three ways to misread the panel: the pre-read is 43-55 tips and its matched-wall recovery is least likely to hold on the 74-88 tip matrices where the fixed-replicate gap was largest (5 steps on Zanol2014 vs 1-2 there), so a size-conditioned default is the likelier honest outcome than a global flip; tabuSize = 0 also disables the tabu list, so those rows bound the lever's size rather than describing `default`-preset behaviour; and nEvsImproved is NOT "improvers the gate cost you" -- what certification would have found in a run that never ran it is unobservable by construction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Read order matters: a matched-wall budget warning invalidates the *_mw rows, and nEvsSkipped == 0 means an arm never fired, so both must be checked before the floor-attainment table is interpreted at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Relative `@` imports in a CLAUDE.md resolve relative to the directory of the importing file, not the project root. `.claude/CLAUDE.md` containing `@AGENTS.md` therefore looked for `.claude/AGENTS.md`, which does not exist, and a missing import target is silently ignored -- so AGENTS.md was never loaded into any session and the literal string `@AGENTS.md` appeared in context instead. Verified fixed in a fresh session on a sibling repo: the pointer shows `@../AGENTS.md` and AGENTS.md content is present. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…usting the filter 7685bf0 closed the T-364/T-370 leak; this adds the check that would have made it loud rather than silent, for whatever causes it next. `constraint_fallback` was never that check. It only fires when the placement filter rejected *every* edge, which the leak never did -- it returned violating trees mutely, so the documented warning never once fired in 736 measured calls. Nor does the caller's post-hoc retry cover it: `build_constraint_posthoc()` is called only from the search entry (ts_rcpp.cpp), so `AdditionTree()` runs with `has_posthoc = FALSE` and has no reshuffle to fall back on -- including for the both-sides-straddle case that wagner_collect_active_splits() explicitly defers to "the post-hoc check". So verify the finished tree directly: a bipartition is displayed iff some non-root node's subtree tip set equals one side of it. Orientation-agnostic, so it stays correct however the tree ends up rooted -- including after 796a29d's reroot, and for any future producer that roots differently. Report it through WagnerResult and warn from the R thread. Rf_warning() was being called inside the Wagner kernel, which also runs on search worker threads where that is not safe; this removes the hazard rather than widening it. It also surfaces random_wagner_tree()'s 100-attempt retry exhaustion, silent until now. Cost is one postorder scan per finished tree, O(n_node x n_words x n_splits), against a build that is already O(n_tips) postorder scans. Validated on the merged fix: 1334 randomised constrained cases (6-45 tips, 1-4 groups, nested constraints included) -- 0 violations, 0 warnings, 0 spurious. With complement enforcement disabled, 425 of those 1334 violate and the check caught 425/425, still with no false positive on the other 909. The added test sweeps for silence, since spurious warnings are the realistic failure mode of adding a verifier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ly TBR/SPR screen Ports a fresh oracle (mirrors dev/benchmarks/tbr_oracle.R's bestImproving() shape, scored under TreeLength(..., inapplicable="hsj"/"xform") instead of Fitch) that asks the question T-377 actually needs answered: does the Fitch-only candidate screen (ts_tbr.cpp:2206, fitch_indirect_length_cached) cause premature termination (a real improving TBR neighbour left unexplored), not just a different ranking of improving moves (which costs nothing, since the accept gate is exact per T-306). Maximal-effort MaximizeParsimony() convergence (ratchet+drift+sector, what every shipped strategy actually runs) is clean in all 36 sampled runs across a mixed-weight and a near-blind (single free character) 8-tip HSJ/XFORM matrix. A bare hill-climb (no ratchet/drift/sector) isolates the mechanism as real: 2/10 runs on the near-blind matrix under HSJ converge one step short of a genuine neighbour. So the blindness is real but fully absorbed by rescue mechanisms every real search invocation already runs.
Records the d48e0980 measurement result in the findings row: the Fitch-only TBR/SPR screen's blindness is real (confirmed via a bare hill-climb on a near-blind matrix) but fully absorbed by ratchet/drift/sector at every shipped search strategy (0/36 premature terminations at maximal effort). A hierarchy-aware screen would need a full_rescore per candidate (no cheap incremental hierarchy delta exists) for a benefit this measurement cannot detect -- not adopting a fix.
pruneReinsertCycles > 0 under a topological constraint could silently accept a constraint-violating tree: the reduced-tree TBR (step 4) runs with cd = nullptr, and expand_and_reinsert()'s greedy Wagner placement never reads cd at all, so a cycle can return a strictly-better-scoring but constraint-broken tree that the full-tree TBR polish (step 6) can only refrain from worsening, not repair. Step 7's accept/revert now also verifies constraint compliance via map_constraint_nodes() before capturing a cycle's result, mirroring the accept gate in ts_nni_perturb.cpp:117-123. Verified against a pre-fix throwaway worktree build (reproduces 0/2 compliant, score 193 vs constrained optimum 207) and the post-fix build (207, 12/12). New regression test in test-ts-prune-reinsert.R; test-ts-constraint-multi/-small/-rooting, test-ts-impose-constraint, test-ts-driven all pass post-fix under NOT_CRAN=true. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ttributed Validates the merged T-364/T-370 fix (7685bf0 + 796a29d) on the 25-matrix MBANK_FIXED_SAMPLE (training split only), against its pre-fix parent bbcca1b AND against the intermediate 7685bf0, which has complement enforcement but no re-root. All three arms predate the T-384 fix so the defect is still live in arm 2. Three arms interleaved on ONE node per cell, order rotated by task id; 423 runs, zero censored. Q1 — the merged fix costs nothing. Wall arm3/arm1 median 1.000 (22 slower / 22 faster / 3 tie, 8 of 47 >10% slower but balanced by 9 >10% faster, sign p = 1); at the production maxReplicates = 96, 0.967. Score +0, 2 worse / 2 better / 43 tie. Median + sign count + count >10% slower, never the mean. Q2 — the "~1.43x" is REATTRIBUTED, not retracted. It was a real cost of a real configuration, complement-enforcement WITHOUT the re-root, misattributed to the fix and to accidental Wagner restarts. Decisive: arm 1's violating set and arm 2's complement-rooted set are the SAME SET — identical medians and ranges over 21 matrices, and on the T-364 test case the recorded 35/400 = 8.75% reproduces to the digit with arm 2's complement-only rate also 35/400 on the same orders. An order that used to skip the constraint ran an effectively unconstrained search; the same order now runs a fully move-blocked one. That needs no restarts, which matters because AdditionTree() has has_posthoc = FALSE and the Wagner build is ~1-2 ms. Magnitude grows with the budget (in0 1.010 at 24 replicates, 2.531 at 96; p = 0.022), which is the signature of budget exhaustion rather than per-unit slowness, and the re-root removes it (0.382, p = 0.0034). At a fixed budget the same defect shows up as SCORE instead (6 of 22 worse, 0 better, p = 0.031) — the old number was measured as "wall to match the old score", so this reconciles it rather than contradicting it. Also corrects T-384's recorded diagnostic signature, which was flagged "worth reusing": "every phase slower on a score-identical trajectory" has its SIGN FLIP with the replicate budget on identical code (arm 2 cumulatively faster in 6 of 7 phases at budget 24, slower in 6 of 7 at 96). Phase totals sum over replicates and the replicate COUNT is what the defect changes. The invariant that does hold at both budgets is TBR work per replicate roughly halving. Constraints are generated once against arm 3 and frozen, so no arm is advantaged by a constraint it helped choose; every one has a strictly positive score penalty (+3 to +66) rather than merely being absent from one MP tree; and they are stratified on which side holds dataset tip 0, which is what decides whether arm 2 can fail at all and is plain matrix row order, not a user choice. Only this commit's own edits to findings.md are included; a concurrent session's in-progress T-384/T-390/T-391 work in that file remains uncommitted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
sprFirst = TRUE's SPR warm-up pass had no ConstraintData parameter at all, so under a constrained search it could accept a regraft that violated the constraint. TBR afterwards cannot repair a violated constraint (regraft_violates_constraint rejects all moves once a split is unmapped), so the violating tree's improved score carried through unchallenged. spr_search() now takes an optional ConstraintData* cd = nullptr; each accepted regraft is verified via map_constraint_nodes() before being kept, mirroring the accept gate in ts_nni_perturb.cpp:117-123 and tbr_search's own post-hoc validation. ts_driven.cpp now passes cd through to the sprFirst warm-up call. Verified against a pre-fix throwaway worktree build (reproduces the finding exactly: score 193, 0/1 compliant vs. the constrained optimum of 203, 100/100 compliant) and the post-fix build (203, all compliant). New regression test in test-ts-constraint-multi.R fails pre-fix and passes post-fix; test-ts-constraint-multi/-small/-rooting, test-ts-impose-constraint, test-ts-driven, and test-MaximizeParsimony-features all pass post-fix under NOT_CRAN=true. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
feat(effort): scale the MPT-enumeration ceiling, not poolMaxSize
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
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>
Sync cpp-search from agent-issues (post-split reconciliation, pre-resolved)
`normalize` becomes `chanceCorrect` in `ClusteringConcordance()`, `QuartetConcordance()` and `ConcordanceTable()`. The argument moves the zero point to the value expected under a fixed-marginal null; the scaling to a maximum of 1 was never governed by it, so the old name described something the argument does not do. `unit = "trit"` becomes `unit = "nrqs"`, after the quantity it counts: non-redundant quartet statements. Of the quartets a split of sizes (k, t - k) resolves, (k - 1)(t - k - 1) are non-redundant under the Nelson-Ladiges entailment. `QuartetConcordance()` chance-corrects by default, as `ClusteringConcordance()` already does, so values may be negative where agreement falls below chance expectation. `chanceCorrect = FALSE` gives the uncorrected measure. Documentation gains the caveat that the NRQS ceiling of 1 bounds each split individually, so it holds for `return = "edge"` but not for `return = "char"`, which averages over every split in the tree. # Conflicts: # NEWS.md
`ms609/MaxMin` is now `ms609/Coreset`; every exported solver keeps its name, but the progress option moved from `MaxMin.progress` to `Coreset.progress`. `WideSample()`'s dispatch, its dependency guard, the `Suggests`/`Remotes` entries and the Shiny app's option are updated accordingly, as is the `skip_if_not_installed()` guard in `test-WideSample.R` -- which had been skipping silently, since `MaxMin` can no longer be installed under that name. Three references stay as they are: `ExactMaxMin()` is a function name, the "MaxMin optimum" is the objective rather than the package, and `inst/REFERENCES.bib` holds the title of the cited paper.
`spell_check_package()` flags four technical terms, all legitimate: the Nelson-Ladiges surname and the NRQS acronym arrive with the concordance rename, while `multistate` and `unclamped` predate it and were already failing the check.
The argument was described in both `@details` and `@param`, and the two disagreed on what an integer samples: the details said `n` uniformly random trees, the parameter entry said `n` reshuffles of the character's tokens. Both are true, of different functions. The one surviving statement sits under `@param`, and names the null each function actually draws from: `ClusteringConcordance()` scores each character against `n` trees from `RandomTree()`, whereas `QuartetConcordance()` permutes each character's tokens across the scored leaves, holding its state frequencies and the split sizes fixed -- the same fixed-marginal null that its `chanceCorrect = TRUE` expectation evaluates in closed form. `man/SiteConcordance.Rd` is updated to match.
Four files needed resolving. `cpp-search` had already followed the MaxMin -> Coreset rename (089b851), and gone further: `FarFirst()` is called with named arguments, and `Remotes:` has become `Additional_repositories:`, which is what lets pak resolve the package on the Windows CI job. Its versions win throughout; the duplicate `Coreset` entry in `Suggests` is dropped. `cpp-search` also renamed `QuartetConcordance()`'s `return` options from `c("character", "site", "default")` to `c("edge", "char")`, matching `ClusteringConcordance()`, and validates them rather than falling back on a default. The NRQS path adopts that vocabulary: `return = "default"` becomes `return = "edge"`, and the validation is hoisted above the `unit == "nrqs"` branch so that path receives a checked value. The chance-correction block keeps its place, and `miRand` is now read through `.ConcSlice()`. Two tests were left stale by the merge, both now updated: - the `return` alias assertions asserted that `"default"`, `"character"` and `"site"` are accepted, which the argument-checking test on the same file now asserts they are not; - the `{0-}` ambiguity test bounded results to [0, 1], which no longer holds now that `QuartetConcordance()` chance-corrects by default. The ceiling of 1 still stands, so the lower bound gives way to a finiteness check. `test-Concordance.R` and `test-WideSample.R` pass in full against a build of the merged tree; `spell_check_package()` is clean.
feat(concordance): Nelson-Ladiges "trit" currency and chance correction for QuartetConcordance
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
3 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.
Manual testing underway; shiny app in particular has some usability issues.