C++ search - #210
Draft
ms609 wants to merge 1246 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
…cters
PrepareDataProfile()'s early return set info.amounts to a bare double(0)
instead of a matrix, so scoring a multiPhylo under profile parsimony
against an uninformative character failed converting it to a C++
NumericMatrix ("Not a matrix."). Fixed to emit a zero-column matrix.
LengthAdded()/PolEscapa() crashed the same way for the same input
("argument is of length zero"), since it reassigns char to the
zero-character prepared dataset and then indexes it per leaf. Guarded
with an early all-zero return: ambiguating one already-uninformative
leaf cannot create information.
Adds regression tests for both TreeLength(multiPhylo, ...) and
LengthAdded(), and updates the PrepareDataProfile() empty-matrix
fixture. Marks T-372 resolved in dev/red-team/findings.md.
Follow-up to 323a0da. The values shipped there were measured entirely with nThreads = 1, and the parallel path is not equivalent -- so the non-zero default landed on a path that had not been executed once. Now exercised, with two things the run falsified: - The parallel rule counts a dry spell over replicates completed into the shared pool and is evaluated on the coordinating thread's 200 ms poll, so it fires later and less precisely than the serial rule: patience 5 on one 40-tip matrix stopped at 6 replicates serial, 21 on two threads, 65 on four. The `lastImprovement + patience` identity is serial-only, and `last_improved_rep` is 0 in parallel anyway, so the ?MaximizeParsimony sentence pointing readers at that arithmetic now says "in a serial search". Documented in ?SearchControl and NEWS; noted at the call site that the wall saving will be smaller in parallel. - If replicates are cheap enough that the whole search finishes between two polls, the rule never runs at all: the monitor wakes, sees `replicates_done >= max_replicates`, and breaks. That granularity is shared by perturbStopFactor and consensusStableReps -- it is a property of the parallel monitor, not of this rule -- and it is why the new test uses a 40-tip matrix rather than the 8-tip one the serial tests share. No behaviour change; the parallel implementation is as landed and does not crash at 2 or 4 threads (worth checking given PR #258's MinGW heap corruption on this path, and a restructured guard in the replicate loop). Also pins `strategy = "none"` in that test, since 40 tips x 30 characters makes `auto` resolve to `default` -- whose new implied-weights patience stopped the control arm and made the comparison measure nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Merged rather than rebased deliberately: 34 local commits from several parallel sessions cite each other's SHAs inside dev/red-team/findings.md and log.md, so a rebase would invalidate those citations and force the same two files to be re-resolved up to 34 times. Precedent: 08a3820. Conflict resolutions (all four were additive; nothing was dropped): - dev/red-team/log.md -- kept the incoming T-366 directed-fix entry and set last_focus: 10. The merge base and remote both read 8; the remote correctly left it alone because its entry was a directed fix, not a rotation round. Local had advanced 8 -> 9 (area-9 round) -> 10 (area-10 round), so 10 stands. - dev/red-team/findings.md -- BOTH SIDES HAD FILED A T-366 ROW for the same defect at the same file:line: the area-9 round filed the diagnosis, a directed fix session filed the fix. Collapsed into one row combining the area-9 row's mechanism trace (ts_fitch_na.h:43-68, the ts_sector.cpp sibling comment, the authority sites) with the fix row's applied fix, Dikow2009 A/B and "fixed d94d76b". Verification status is now stated precisely because the two rows disagreed: the FINDING was opus-verified; the FIX was not independently re-verified. Heed red-team's own advice to leave an ID gap when sessions run in parallel. Dropped the incoming T-362/T-363 rows as strict subsets -- the local versions add the "Resolved:" sections recording that both were fixed. - R/AdditionTree.R + man/AdditionTree.Rd -- both sides rewrote the same @return block to correct the same rooting claim. Combined: local's fuller content (degree-two root, the rooting-invariance rationale, the <4-taxa clause) with the incoming link form (\href to the TreeTools reference page). Also narrowed T-380 in light of what came in: its unpack_xform tip_states half duplicates the already-filed T-344, now fixed by 70ae4d5. That fix guards only the tip_states length in unpack_xform -- it does not validate cost-matrix dimensions anywhere and does not touch ts_sankoff_test, which is what T-380 now covers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tree_fuse() had zero R_CheckUserInterrupt()/check_interrupt() calls in its round/donor loops, unlike every other search module. For a large pool-wide fuse (fuseInterval, sprint default 5) over a >100-tip pool, the O(n_donors * n_splits^2) donor scan could run for many seconds between other checked points, delaying Ctrl-C/Esc response well past the ~1s target reported by the user. Add ts::check_interrupt() at the top of the round loop and per-donor, which correctly maps to R_CheckUserInterrupt() (serial) or the shared atomic stop flag (parallel workers), matching the pattern used elsewhere (e.g. ts_ratchet.cpp, ts_tbr.cpp).
A user watching a 182-tip, 420-character matrix with inapplicable tokens throughout reported the search looked frozen. It was not. Measured, one sprint replicate takes 1173 s, of which TBR is 582 s and a 3-cycle ratchet 549 s -- 96% of the replicate in two phases that printed only on completion. Nothing reported progress for ~10 minutes at a stretch, and the finest granularity available was one line per replicate, i.e. one line per 20 minutes. Heartbeat. Long phases now report their running best and time in phase, overwriting a single line at a terminal and emitting discrete lines to a batch log. Cadence defaults to 30 s / 120 s respectively, override or disable with TS_HEARTBEAT_SECONDS (fractional values allowed; junk falls back to the default rather than disabling). Reporting is opt-in per call site via TBRParams::heartbeat_label, not opt-out. A caller is the only thing that knows whether a search's running best is on the user's objective: sectorial searches score a subtree, and the ratchet's perturbation phase scores a reweighted matrix. Both legitimately run far below the true optimum -- observed at 33 where the optimum was 79 -- so reporting them would read as erratic progress. Only whole-tree, real-weight searches are labelled, so any score printed is comparable with the final tree score. Thread safety: every entry point no-ops unless ts::thread_stop_flag is null, i.e. on the R main thread, so Rprintf is never reached from a worker. State is plain globals rather than thread_local, given the MinGW emutls corruption fixed in PR #253. The parallel path instead gains a wall-clock cadence on its existing coordinator poll, which was already the only thread touching R there, and which previously reported only when a replicate completed -- silent for hours on a large matrix, and indistinguishable from a hung job. Cost: the TBR hook piggybacks the existing per-clip interrupt poll at stride 64 rather than adding a clock read to the candidate loop, which would have reopened a hot path the profiling campaign left at-limit. The ratchet hook uses stride 1, since a cycle is coarse and a larger stride would silence it entirely on the deep ratchets implied weights asks for. Strategy partial matching. `strategy = "thoro"` previously warned "Unknown strategy" and silently fell back to `default`. Any unambiguous abbreviation now resolves; every preset's initial differs, so one letter suffices. Resolution happens BEFORE the preset lookup and before .IwRatchetDepth()/.IwStopPackage(), which test `strategy %in% c("thorough", "large")` by exact string -- resolving later would apply thorough's preset while skipping its implied-weights ratchet package, a worse failure than not matching because it looks like it worked. pmatch() rather than match.arg() so a real typo still warns instead of erroring. Also documents TREESEARCH_PROGRESS_FILE, which already existed in R/, the Shiny app and dev notes but appeared nowhere in man/ -- the one mechanism that gives a non-interactive run any progress at all, and it was undiscoverable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…y are swept
findings.md cited both P1 reproductions by session-temp paths
(...\scratchpad\V7.R, a1001d.R). That directory gets swept, and the skill's own
tidy doctrine says artifacts backing an OPEN finding are live and must never be
swept -- so the two load-bearing scripts are now in the repo, re-run, and
confirmed to still reproduce the verifiers' exact numbers:
- hsj-token-permutation.R (T-376, from V7.R)
sweep 1 (zero secondaries, so fitch_label_char is provably never entered)
gives HSJ 2 or 3 for one identical dataset+tree; sweep 2 gives 1, 2 or 4;
BGS control constant in both. Documents why permuting matrix ROWS is the
wrong experiment (pick_state's tb_mintip tie-break makes row order change
tip numbering), and asserts PhyDatToMatrix() byte-identical per iteration.
- hsj-totalwords-zero-noop.R (T-373, from a1001d.R)
zeroW returns the start tree bit-identical under maximal effort (10 -> 10)
while the one-non-zero-weight control moves it (13 -> 8). Documents why the
tempting "returned 8, sampled 6" comparison is confounded by rooting.
Exits 2 INCONCLUSIVE if the CONTROL fails to move the tree, so a broken
harness cannot masquerade as a confirmed bug.
Both take an optional library path as argv[1], matching hsj-paper-oracle.R, and
exit non-zero while their bug is live so they work as fix gates.
Scripts NOT ported are named explicitly in their finding rows (V2/V3/V4 layout
and 57%-invariant sweeps; a1001e/f/h bootstrap reachability and topology
enumeration) -- their results are recorded, but re-deriving them needs fresh
scripts rather than a rerun.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`R-CMD-check.yml` runs covr/codecov only in Wave 2, which is gated on `sense-check`; sense-check has been failing on cpp-search, so Wave 2 was skipped and coverage has not run on recent work. `tests/spelling.R` is one of the two reasons. Eight of the nine words are legitimate technical vocabulary and go into inst/WORDLIST: pectinate, preset's, reweighting, run's, segfaulted, trifurcation, unparseable, unrooting. Possessives match the existing convention there (dataset's, replicate's, TNT's). "erroring" is not a word, so it is reworded to "throwing an error" rather than added -- once in the `strategy` roxygen (regenerating man/MaximizeParsimony.Rd) and twice in comments, so the same non-word does not sit in the source waiting to reappear in a future .Rd. The pre-existing sort disorder at inst/WORDLIST:138 (RAS after RJ) is left alone: it is not new, and reordering adds diff noise to a file that concurrent sessions also touch. Verified: tests/spelling.R, run from tests/ as R CMD check runs it, reports "All Done!"; spell_check_package() finds no errors. This does NOT make sense-check green on its own -- test-AdditionTree.R:93 still fails (open bug T-364) and is untouched here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nt misscoring T-380 (ts_sankoff_test dimension guard): Add checks to ensure cost_matrices and tip_states_r have correct dimensions before accessing them. Missing dimension validation could silently read wrong values due to unchecked matrix indexing. T-381 (min_steps invariant violation): Replace silent clamp with error when precomputed_steps > min_steps_r. This invariant violation indicates a bug in simplify_patterns or invalid user input; raising an error prevents silent IW score inflation. Tests added: - test-ts-sankoff.R: 5 new tests for cost matrix and tip_states dimension guards - test-t381-min_steps-guard.R: 3 new tests for min_steps invariant validation - test-ts-simplify.R: Fixed one test that was passing invalid min_steps (now uses default instead of explicit zeros) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Mark T-380 (ts_sankoff_test dimension guard) and T-381 (min_steps invariant guard) as FIXED by commit 0856748. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…+ min_steps invariant guard)
…s (T-382) compute_collapsed_flags's condition 1 read tree.local_cost for every standard block unconditionally, but fitch_downpass (ts_fitch.cpp:85) skips blocks with active_mask == 0, leaving their local_cost stale. The stale value only ever made condition 1 fail (under-flagging collapsible edges), never caused a spurious collapse, so this was a lost optimisation rather than a correctness bug. Mirror the active_mask == 0 skip that ts_fitch.cpp and the already- correct compute_collapsed_flags_aggressive both use. Left the full-row memcmp (condition 3) as-is with a comment recording that its staleness is one-sided-safe, since masking it per-word wasn't judged worth the complexity for a P3. Verified the MPT set returned by MaximizeParsimony() on a real, ratchet-heavy run (congreveLamsdellMatrices[[1]]) is unchanged before and after the fix (same score, same split-set).
…apse kernel) # Conflicts: # dev/red-team/findings.md
…t the NA wall is Two changes to the exact_verify_sweep subsystem: a latent cache-key defect, and the instrumentation that found out where native-inapplicable search actually spends its time. weight_fingerprint() keyed the exact_verify_sweep optimum cache on active_mask, upweight_mask and pattern_freq -- the only scoring state that varied for a given DataSet when it was written. Concavity and scoring_mode were omitted, so before this change k=10 and k=40 hashed identically (2132fc2543dc37a9). Nothing on the current default path scores one DataSet under two regimes, so this was latent rather than live; anything that does -- a concavity sweep, or a phase swapping in a cheaper surrogate scorer -- would have had a "genuine optimum" verdict recorded under one regime served to the other, suppressing a real improving move with no symptom but a worse tree. Both fields are now mixed in, pinned by a new test-ts-na-evcache.R case that fails without them. TS_NA_TIMING (default off) decomposes tbr_search into disjoint brackets -- exact_verify_sweep, the below_actives_cache build, compute_from_above + vroot, the accept-path rescore, and the remainder -- accumulated onto the DataSet so one call yields whole-search totals, and exposed on ts_tbr_search and ts_ratchet_search. The clock is sampled only when the flag is armed: three of these sites are per-clip or per-accept, where an unconditional steady_clock::now() would tax the default path. A positive remainder is the check that the brackets do not overlap. What it found, over four matrices spanning 7-64% inapplicables and 23-88 tips: exact_verify_sweep is 97.7% of native-NA tbr_search wall (89.4-99.6% individually), while the per-clip scaffolding is 0.08% and the accept path 0.4%. That reverses the standing diagnosis, which named below_actives_cache as the first suspect -- it is 0.001 ms per build. The sweep never touches n_candidates_evaluated, which is why candidate-based metrics measured the ~2% and were blind to the rest. Its memo cache hits 2.1%: it stores only true-optimum verdicts, and a still-improving search does not revisit the same (topology, regime) pair. Details and both candidate levers in dev/profiling/na-exact-verify-dominates.md. No default behaviour change beyond the cache-key fix. 866 assertions green across the NA, TBR, drift, sector, ratchet, IW and parallel suites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nrooted
Investigation only; no src/ change.
Decision: do NOT pin a rooting for XFORM. Three results, all in
dev/plans/2026-07-29-t374b-xform-rooting-policy.md and backed by the new
pure-R oracle dev/red-team/heavy-tests/xform-rooting-oracle.R (no build
needed; exits 0 while its stated predictions hold):
1. The cheap fix is REFUTED. forced_root_state = 0 ("absent at the root")
makes rooting-dependence far worse — 84-117/120 random 9-tip topologies
vs 0-34/120, spread up to 5 vs <= nSec — and makes even the *symmetric*
nSec = 0 control rooting-dependent (0/120 -> 84/120). Pinning the root
state does nothing about the root position and removes the
min-over-states slack that was absorbing it.
2. The dependence is bounded and one-directional. The asymmetric part of
the x-transformation cost matrix is a gradient, so the root is charged
f twice; tested spread bound is nSec per block (an earlier nSec/2 guess
was falsified — rooting subdivides an edge). An arbitrary rooting always
OVERstates the min-over-rootings objective; 87-98% of rootings attain
it. So the search optimises a sound but loose upper bound.
3. TBR is not the obstacle. TBR's fragment reroot is an unrooted-topology
change, not a rooting change, and the default path never physically
reroots the whole tree. Whole-tree rerooting is four enumerable sites.
Recommended fix is a reporting fix: make MaximizeParsimony's reported
score and TreeLength() agree on one rooting, and document XFORM as
rooting-sensitive. A/B verdict: do not run the rooting-pinned A/B.
Also files T-383 as a candidate (unverified, code-read only): the Sankoff
term is absent from XFORM's TBR candidate screen, so move selection is
Fitch-guided, and whether the convergence sweep sees the term depends on
whether non-hierarchy characters happen to carry gaps. First-order, and it
must be settled before any rooting measurement is interpretable.
Review caught an over-claim in the previous commit. The Q-C measurement compared spread-across-rootings of a root-FREE criterion against that of a root-PINNED one. Those are different criteria, so the comparison cannot support "pinning the root state is strictly worse". The nSec = 0 pinned cell (84/120) was cited as decisive and is in fact a tautology: constraining the root's label on a symmetric matrix must make the root position matter. The recommendation is unchanged and unaffected — it rests on Q-E (an arbitrary rooting always OVERstates the min-over-rootings objective; 87-98% of rootings attain it) and on the T-383 blocker, both sound. Only the argument changes: - Option 1 is rejected as INCOHERENT ON ITS OWN, not as harmful: pinning the root state makes the root meaningful while leaving its position unchosen and, at four sites, moving, so the score comes to depend on a root the user never selected. It is only meaningful together with pinning the position, i.e. it is Option 2, not a one-liner. - Q-E promoted to lead evidence in the recommendation. - The nSec = 0 row is now marked as an asymmetry control for the FREE arm only, and the two arms as non-commensurable. Oracle gains the discriminating control: the pinned-state score is verified reproducible at a fixed canonical rooting, making explicit that the pinned criterion is well-defined and its spread is purely the unchosen-root cost. Still exits 0. Also notes that T-383's number was claimed as max+1 and may collide with a concurrent session, so a tidy pass can renumber freely.
…new finding
Caught before landing. The hierarchy-blind TBR candidate screen I derived
from code and filed as T-383 is T-377, "TBR's candidate scan is
hierarchy-blind, so moves that improve the HSJ DP or Sankoff term at a
cost in Fitch steps are never proposed" (P2, verified, ts_tbr.cpp:2206) —
and it is being worked on concurrently in claude/t377-hierarchy-screen.
- Delete the duplicate T-383 row.
- Fold the three genuine increments into T-377's row instead:
(a) "seen solely by the accept-time full_rescore" is incomplete and the
gap is has_na-CONDITIONAL (ew_directional at :1739); under XFORM
has_na is data-dependent, since non-hierarchy characters keep their
gaps, so search behaviour changes with an incidental property of the
data;
(b) the `dominated` test is systematically over-permissive — Fitch-only
candidate vs Fitch+Sankoff incumbent — a wall-clock cost T-377 did
not record, and it makes the per-clip argmin the Fitch argmin;
(c) T-377 gates T-374's XFORM A/B.
- Repoint every T-383 reference in the T-374 row and the decision document
to T-377, and note the concurrent worktree so this is coordinated rather
than duplicated.
No change to the recommendation: it rests on Q-E and on T-377, both intact.
XFORM's `-2` tip-state sentinel ("present, some secondary unknown")
previously freed every present state regardless of any secondary that
WAS observed, discarding real information and biasing the search
toward zero-cost placements.
RecodeHierarchy() no longer breaks out of its per-secondary loop on
the first unknown token; it now records, per tip, which secondaries
were resolved (`tip_sec_known`) alongside the block's existing
`combo_grid`, and exports both on the block list. Both C++ consumers
of the `-2` sentinel -- unpack_xform (the live ts_driven_search /
ts_collapse_pool path) and ts_sankoff_test (the TreeLength()/test-only
kernel, via two new optional trailing params defaulting to R_NilValue
for backward compatibility) -- now intersect against these to build
the admissible state set instead of freeing everything.
Chose to pass per-secondary tokens through and let C++ build the
admissible set (rather than enriching the sentinel itself), since the
combinatorics don't fit in one spare integer once more than ~5
secondaries are in play. Left ts_data.cpp's token_states (the T-375
plumbing) untouched -- XFORM's secondaries live in the block's own
R-side level space, not the Fitch contrast-token bitmask T-375 stores,
so there is nothing to share yet.
Adds a regression test showing a 2-known/1-unknown tip whose known
secondaries conflict with a fully-resolved comparison tip now scores
strictly more than the old free-ride of 0, on an isolated 2-tip tree
(a 3rd tip needed only to register a secondary's second informative
level is excluded from the scored tree, since a 3-tip topology's
Hamming geometry can mask the effect entirely).
Gotcha: ts_sankoff_test's signature is manually mirrored in
src/TreeSearch-init.c; Rcpp::compileAttributes() does not touch that
file, so a param-count change needs a matching manual edit there too.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…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>
Pasting the tree key and the state counts into one comma-delimited string left no marker for where the first ended and the second began, so distinct (tree, counts) pairs could render identically -- "100" + "1,2" + c(3, 4) and "100" + "1,2,3" + c(4) both give "100,1,2,3,4" -- and return a false cache hit. A tip label containing a comma collided the same way. Nesting a per-character environment inside a per-tree one removes the concatenation altogether. It also stops the tree key, whose length grows with the tree, from being copied into every character's key: a 20-tip tree now stores one 292-character key beside 13-character per-character keys, where before each character carried the whole thing. Also document that rhi is NA when nRelabel is 0, which the return block omitted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ci: add libstdc++ hardened-assertions leg to agent-check.yml
…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
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)
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.
Manual testing underway; shiny app in particular has some usability issues.