C++ search#210
Draft
ms609 wants to merge 877 commits into
Draft
Conversation
ms609
added a commit
that referenced
this pull request
Mar 28, 2026
ms609
added a commit
that referenced
this pull request
Mar 28, 2026
Set rogueInstalled in the load-trees chunk. Wrap the entire exploring-consensus chunk body (Rogue feeds plottedRogue/consTrees/ tipCols which all feed RoguePlot, so they must be gated together). In cluster-consensus, gate only the two Rogue lines inside the loop and substitute character(0) / black tip colours as fallbacks so the consensus plotting continues without rogues. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Accept fractional per-character weights via attr(dataset, "weight")
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>
…taset S-RED area-5 finding: ts_sector.cpp's build_reduced_dataset() did not copy flat_blocks or all_weight_one from the full DataSet. Since use_flat = ds.all_weight_one is checked at the top of every tbr_search() call, sector TBR always used the slower non-flat path even when all characters have weight=1. Fixing all_weight_one without flat_blocks would have triggered UB (flat functions dereference flat_blocks.data()); both fixed together. Also: - profiling round 1: Ratchet area (#2) PROFILED — 62% of search time via verbosity=2 phase data; VTune not available for per-function hotspot; T-300 (lazy rescore) is the actionable follow-up - focus-areas.md: NNI-perturb → AT-LIMIT (disabled via T-274), Ratchet → PROFILED with 2026-05-18 baseline (2.80 s/rep Zhu2013 thorough) - red-team.md last_focus → area 5 with full findings 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>
Replace full_rescore() on the accept path with two incremental downpass+uppass pairs from nz (clip grandparent) and nx (spare/regraft node) — the two stale chains after apply_tbr_move in the SPR case. TBR moves (with non-trivial rerooting) still fall back to full_rescore. DEBUG_RESCORE guard enabled: asserts incremental == full_rescore on every accepted SPR move. Remove #define once GHA passes cleanly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ash on linux Incremental SPR rescore systematically underestimates score by 3 steps (observed on ubuntu GHA; diff=-3 consistent across all test cases). Root cause not identified; full_rescore restored as the safe fallback. T-300 to be redesigned with per-node local_cost instrumentation offline. R_Interactive guard in ts_parallel.cpp is kept (separate fix, working). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Four tests in test-ts-stopping.R and one in test-ts-xpiwe.R loaded inapplicable.datasets but then used inapplicable.phyData, a different object. Tests only passed because a prior test had already loaded inapplicable.phyData into the environment. Changed to load the correct object (inapplicable.phyData) so tests are self-contained. Found by red-team area #8 review. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ailure ts_drift.cpp:680 — second drift_apply_tbr_move call in the EW RFD re-apply path lacked drift_restore_topology() before build_postorder() on failure. Mirrors the correct pattern at line 589. Path is theoretically unreachable (same args succeed on first apply → succeed on second), but guards against partial step-1 detach leaving an invalid tree if the path were ever reached. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace full_rescore() on the SPR accept path with a single-pass postorder walk over the union of paths nz→root and nx→root. Each affected node is visited exactly once, reading current children's prelims — no shared- ancestor double-read, no IW arithmetic on stale state. This supersedes the reverted overlap-chain implementation (1e3fc9a), which underestimated EW length by exactly 3 steps on every accepted move. Root cause of the −3 was never identified analytically; the dirty-set algorithm sidesteps the question by construction. New functions in ts_fitch.cpp: fitch_dirty_downpass(tree, ds, start_a, start_b) — postorder rescore over the dirty set; returns EW length delta. fitch_dirty_uppass(tree, ds, start_a, start_b) — companion final_ pass, seeded from the same start points, propagating downward. Wired into tbr_search at ts_tbr.cpp:1138, gated on is_spr && !has_na. TBR moves with non-trivial rerooting and datasets with inapplicable characters continue to use full_rescore (NA variant deferred). DEBUG_RESCORE cross-check left enabled: every accepted SPR move asserts incremental == full_rescore. Remove the define in a follow-up after one clean GHA cycle. Local validation (Zhu2013, congreveLamsdellMatrices, synthetic 30×25 EW): - 92 tests passed across test-ts-tbr-search, test-ts-tbr-symmetry, test-ts-ratchet-search, test-ts-ratchet-opt. - 0 DEBUG_RESCORE mismatches across EW + IW paths with multiple SPR accepts per replicate (scores 173, 192, 9.140673). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Profiling round 2 — area #4 (TBR full-rescore at acceptance): - VTune confirmed full_rescore is 19.2% self-time (fitch_na_score 0.585 s + load_tip_states 0.032 s) on Zhu2013 thorough preset, n_threads=1. - All time flows through tbr_search line 1138 (accept path), confirming T-300 as the right target. Zhu2013 has inapplicable characters so the initial T-300 commit (EW only) does not benefit it; an NA variant is the next follow-up. - Secondary candidate identified: ts::simd::any_hit_reduce_avx2 at 9.6%. Adds dev/profiling/drivers/tbr-rescore.R (Zhu2013 driver, ~3.9 s bare). Updates baselines.md, log.md, focus-areas.md, findings.md. RESUME.md refreshed for hand-off after T-300 push. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds a temporary full_rescore cross-check on every NNI incremental delta, gated to EW (IW has a separate pre-existing arithmetic bug, see below). Together with DEBUG_RESCORE in tbr_search, this tests whether the fitch_incremental_downpass single-chain primitive is the source of the −3 that took down the previous T-300 attempt. Local validation (Vinther2008 EW + synthetic 30×25 EW, 5 replicates each): 0 DEBUG_NNI_RESCORE assertions on the EW path. Interpretation: the single-chain primitive is correct. The −3 in the reverted T-300 must have arisen from the two-chain composition itself, not from a bug in fitch_incremental_downpass. Both my analytical argument (deltas cancel) and the dirty-set empirical result are consistent with the primitive being sound; some interaction *between* chains (e.g. shared save_node_state ordering, or a read at LCA I haven't isolated) produced the systematic underestimate. The dirty-set algorithm avoids the question by construction. Separate finding while wiring this assertion: nni_search at line 83 computes `new_score = best_score + delta` unconditionally — but `delta` is an integer EW step delta from fitch_incremental_downpass, and `best_score` for IW is a float weighted score. The IW NNI path therefore reports wrong intermediate scores (diffs of 0.01–2.0 against full_rescore in local testing). Impact is bounded — nni_search() ends with a final full_rescore, so the returned score is correct, but intermediate accept/reject decisions under IW use a wrong predicate. Filed as a follow-up to fix via extract_char_steps + compute_weighted_score (same pattern as tbr_search accept path). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The NNI accept-path in nni_search() added an integer EW step delta to a float weighted score whenever ds.concavity was finite, producing wrong intermediate scores under IW/profile parsimony. Empirically, a 30x25 synthetic EW phyDat with concavity=10 triggered 17+ DEBUG_NNI_RESCORE mismatches per replicate (non-integer diffs like 0.96, -1.94); the EW path emitted zero. After fitch_incremental_downpass, tree.local_cost is correct for the whole tree (NNI only changes children at edge c; off-chain nodes retain valid local_cost from the score_tree at function entry). Mirror the T-300 SPR accept-path pattern from ts_tbr.cpp: under std::isfinite( ds.concavity), call extract_char_steps + compute_weighted_score (which dispatches IW vs profile by ds.scoring_mode). EW path keeps best_score + delta. Also remove the EW-only gate around DEBUG_NNI_RESCORE — it was there purely to mask the now-fixed IW bug. Leaving the assertion live on both paths catches any regression in either branch. Local: dev/plans/t300-test.R clean on both EW and IW; full test-ts-(nni|search|driven|tbr) testthat suites pass with no DEBUG_NNI_RESCORE emissions. T-300 follow-up. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two GHA cycles (R-CMD-check 26078069315, ASAN 26078069318, post-NNI-fix combined 26078652587) confirm zero assertions across all platforms on both EW and IW paths. The cross-check scaffolds have served their purpose: the dirty-set rescore in tbr_search and the corrected NNI incremental in nni_search both match full_rescore exactly. Removes: - src/ts_tbr.cpp:23 #define DEBUG_RESCORE - src/ts_tbr.cpp:1166–1176 #ifdef block (full_rescore + actual = ref) - src/ts_search.cpp:20 #define DEBUG_NNI_RESCORE - src/ts_search.cpp:103–119 #ifdef block (save/full_rescore/restore) The actual rescore work (extract_char_steps + compute_weighted_score branch for IW, dirty-set delta for EW) is unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds NA-aware dirty-set Pass 1 + Pass 2 (fitch_na_dirty_downpass and fitch_na_dirty_uppass) mirroring the EW dirty-set, and wires them into tbr_search's SPR accept path under has_inapplicable. Pass 3 still runs over the full tree because it populates internal down2 (read by extract_char_steps) and counts NA-block steps; savings come from skipping Pass 1 + Pass 2 on off-dirty nodes. Bug found during validation: full_rescore for NA EW data routes through fitch_score_ew which adds ds.ew_offset (topology-independent step count) on top of fitch_na_score's return. My accept path was reading fitch_na_pass3_score directly without the offset — producing a systematic diff=−3 on Vinther2008 (where ew_offset = 3). Fix: add ds.ew_offset in the EW branch (line 1183). IW path is unaffected because compute_weighted_score handles per-pattern step contributions directly. Validation: DEBUG_NA_RESCORE cross-check kept for one GHA cycle. Local Vinther2008 run, 5 replicates each EW + IW: DEBUG_RESCORE (EW, IW): 0 / 0 mismatches DEBUG_NA_RESCORE (NA-EW, NA-IW): 0 / 0 mismatches Also includes dev/plans/t300-test.R (the four-path test harness) and dev/plans/pr244-examples-hang-brief.md (background-agent brief for the separate PR #244 hang investigation). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Threads ConcordanceTable() into the PaintCharacters example so the two functions appear together in the rendered example block. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…sert (T-266) Lands 41b0d2373's exact edge-set insertion scorer in expand_and_reinsert (replacing the union-of-finals fitch_indirect_length_bounded approximation; mirrors the June directional fix at ts_wagner.cpp), plus branch post-review hardening: 4007f8e port (byte-identical to 41b0d2373); 0fb6de9 thread_local probe accumulators (parallel-search data-race fix, production byte-identical); 98122e5 NA + IW score-soundness regression tests (Vinther2008). The 'NOT for cpp-search until composition (#40)' note in 4007f8e's body is superseded: landing the correctness fix is decoupled from the #40 time-matched A/B. Dormant path (prune-reinsert auto-enables only at nTip>=120 / large preset; no default mission search reaches 120t) so nil current wall-clock risk. Verified: clean consistent rebuild + targeted tests 123 pass / 0 fail (incl new NA/IW soundness + wagner-quality). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drop the non-portable value snapshots from the ViewChars/Distribution AppDriver tests. app$expect_values() embeds rendered-plot output (a data-URI image hash plus float plot geometry) that differs machine to machine even on the same OS, so it could never green on GHA. Replace each expect_values() with wait_stable() (retrying wait_for_idle wrapper) and keep the portable coverage: the savePlotZip download-script snapshots (which encode app state), the interaction path itself, and SearchLog's log markers. Regenerate all download baselines; delete every _snaps/*/*.json. Rewrite test-app-smoke to load a bundled dataset (Sun2018) before asserting "N trees in memory" -- a fresh boot has data-dataSource="file" with nothing loaded, so the old assertion was checking a state the app never reaches. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The testServer module tests drifted from the app code while the suite was CI-unwired: * test-mod-search.R carried a hand-copied SearchConfidenceText frozen at an older signature; global.R's real one grew stopReason/replicateScores (consensus/timeout stop notes + Chao1 coverage note). The module now passes those args, so every observeEvent(input$implied.weights) that calls DisplayTreeScores() errored with "unused arguments" and aborted before completing the T-165 run-stats reset -- cascading into the reset/cancel-file assertions (656-658, 519). Re-sync the stub verbatim to global.R and flag it KEEP-IN-SYNC. * TreeLength()/ScoreSpectrum() are TreeSearch exports but the file only attached shiny, so they were "could not find function". Attach TreeSearch. * The consensus stop-note assertion pinned "consensus stable"; the shipped text is "consensus tree unchanged across recent replicates". Align the assertion to the real output. (See note to maintainer: confirm this wording is intended, not an accidental UX regression.) * test-mod-data.R expected tipLabels() to be NULL with no trees, but mod_data.R deliberately guards `return(character(0L))`. Align the expectation. Verified: test-mod-search PASS=66 FAIL=0, test-mod-data PASS=13 FAIL=0. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ARNs - global.R: consensus stop-note now reads "Search stopped: consensus stable across recent replicates." (terse, descriptive); test-mod-search stub kept in sync + assertion pinned to "consensus stable". - test-mod-clustering.R: library(shinyjs) so the clThresh observer resolves runjs() (was 3 "could not find function" WARNs). - test-mod-consensus.R: suppressWarnings around two isolated-harness artefacts (transient non-numeric signif / length-zero if) irrelevant to the assertions. CI fails on WARN; whole EasyTrees suite now FAIL 0 | WARN 0 | PASS 192. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(constraint): reject impossible constraints, guard collapsed-split OOB (T-329) An impossible (non-laminar) constraint reached random_constrained_tree() unfiltered: .PrepareConstraint() only filtered splits by size, never checked pairwise compatibility. In random_constrained_tree(), a split that loses all its tips to tighter non-laminar splits collapses (split_root = -1); if it still has a strict-superset parent, the parent's child-split loop pushed that -1 as a phantom item, causing tree.parent[-1] and tree.left/right[n_tip-1] out-of-bounds writes. Primary fix: .PrepareConstraint() now validates pairwise split compatibility (disjoint or nested) and stops with a clear error before reaching the C++ builder. Defensive fix: guard split_root[j] >= 0 in random_constrained_tree()'s child-split collection, mirroring the existing root-level guard. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci(ASan): switch mem-check runner to ubuntu-latest (x86) The r-hub gcc-asan container image has no arm64 variant; on ubuntu-24.04-arm the container dies at init before any R work runs (every run since 2026-06-19 failed in ~30-40s). Cherry-picked from TreeSearch main's fix (PR #262) so the T-329 fix can be verified under a working ASan CI. * ci(asan): install phangorn for tests leg, exclude MaxMin from vignettes leg The tests leg only installs hard deps + Config/Needs/memcheck (Suggests are skipped for tests/examples), but tests/testthat/*.R make heavy unguarded use of phangorn::phyDat() etc. -> "there is no package called 'phangorn'". Add it to Config/Needs/memcheck so it installs regardless of leg. The vignettes leg installs all immediate Suggests via a single pak::pkg_install() batch, which resolves plain package names against CRAN/Bioconductor only - it doesn't see the local DESCRIPTION's Remotes mapping. MaxMin is GitHub-only (Remotes: ms609/MaxMin), so that batched solve fails outright with "Can't find package called MaxMin", taking every other Suggest down with it. Exclude MaxMin, mirroring the existing Rogue exclusion: its only use (WideSample()) is requireNamespace-guarded and no vignette/example exercises it. * fix: guard zero-length memcpy in TreeState::load_tip_states Datasets whose characters are all uninformative (e.g. every observed state is a singleton) collapse to zero blocks and a zero-length tip_states vector in build_dataset(). std::vector::data() on an empty vector may legally return nullptr, which memcpy's nonnull-parameter attribute forbids — flagged by UBSan (gcc-ASAN run 28662381835) as a null-pointer violation reached via ts_char_steps() -> init_from_edge() -> load_tip_states(). Skip the copy when there is nothing to copy. Covered by the existing "ConcordantInformation() works" regression test in test-Concordance.R, which already exercises an all-singleton dataset via ConcordantInformation() -> CharacterLength() -> ts_char_steps(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(constraint): use four-gamete split compatibility, not laminar-subset (T-329 follow-up) T-329 (19f4f97) added a laminarity gate to .PrepareConstraint() that tested each constraint character's "1"-side as a full bipartition vs everything-else and accepted only disjoint-or-nested pairs. This rejected SATISFIABLE constraints: it (1) folded wildcard "?" and unconstrained tips into the "0" side though they are free to plot either side, and (2) tested only three of the four split-compatibility cases, missing the case where the two "0" sides are disjoint (the "1" sides jointly cover the constrained tips). The gcc-ASAN CI (run 28662381835) surfaced this on the pre-existing Morphy test and the tree-search vignette "complex-constraints" chunk (ab|cef & abcd|ef, g free) -- both displayable on ((a,b),(d,(c,(e,f))))+g. T-329's own "impossible" example {t1,t2,t3}&{t3,t4,t5} was likewise satisfiable (coexist on ((t1,t2),t3,(t4,t5))). Fix: build a companion 0-group matrix and apply the four-gamete test on both groups (wildcards excluded) -- incompatible iff all four group intersections are non-empty. Valid constraints return to their working pre-T-329 path; genuinely-impossible ones still error. The C++ OOB guard (ts_wagner.cpp split_root[j] >= 0) is the real crash fix and is retained unchanged. Tests: corrected the first T-329 case to a genuinely four-gamete-incompatible constraint ({t1,t2,t3}&{t2,t3,t4}); added a positive regression asserting the non-laminar-but-compatible case is accepted. Verified locally: Morphy suite, vignette chunk, and 446 constraint tests all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(wagner): guard zero-Fitch-words start-tree build (all-hierarchy HSJ/xform) When every character belongs to a hierarchy block, the recoded equal-weights dataset has zero blocks, so DataSet::total_words == 0 and the per-word Fitch state vectors (ds.tip_states, edge_set) are empty. wagner_tree() unconditionally took the address of element 0 of those empty vectors (&ds.tip_states[tip * total_words] == &ds.tip_states[0], and likewise &edge_set[node * tw]) to feed the incremental insertion-cost machinery. That is undefined behaviour (address of a past-the-end element of an empty vector) and aborted under the hardened libstdc++ assertions in the gcc-ASAN CI (run 28662381835, ts-xform group; _GLIBCXX_ASSERTIONS operator[] __n < size()). With no Fitch characters every insertion cost is identically zero, so guard the per-word work behind have_words = ds.total_words > 0: skip the edge-set precompute and evaluate the indirect length as 0, while still running the DFS so constraints are honoured and the first legal edge is chosen. The search then optimises the tree from the Sankoff (xform) / hierarchy-DP (HSJ) term alone. The have_words == true path is behaviourally identical to before. Regression tests (test-ts-xform.R, test-ts-hsj.R) run an all-hierarchy dataset through MaximizeParsimony() so the hardened/ASAN CI covers the zero-words path. Verified locally with a -D_GLIBCXX_ASSERTIONS -O0 -g build: the whole ts-xform group (incl. SK-01, which previously aborted) and ts-hsj group now pass with no abort; a normal equal-weights search is unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…harness Investigates whether the T-327 guard `build_postorder().size()==n_internal` (impose_one_pass, src/ts_constraint.cpp:735) is equivalent to structural validity, and whether a net-zero corruption can slip it to reach the advertised std::bad_alloc from MaximizeParsimony(constraint=). Adds dev/red-team/heavy-tests/impose_validity/ — a standalone C++ harness (no R/Rcpp/Morphy/SIMD) that enumerates all (2n-3)!! rooted binary trees for n_tip=4..8, runs the verbatim kernel functions (topology_spr, collect_edges_*, find_maximal_subtrees, compute_node_tips extracted from HEAD at build time) + the real build_postorder, and models the whole impose_constraint loop. Findings (T-333, P3): the guard is NOT a full validator — it admits reachable net-zero TYPE-1 left/right corruption (double-ref + orphan) that DOES reach the DFS helpers, so its comment is inaccurate. But the hypothesised P1 is REFUTED: 0 root-reachable left/right cycles AND 0 parent[] cycles across ~1.33M accepted-invalid trees (no std::bad_alloc on either vector, incl. the pre-verify tbr_search->reroot_at_tip path), and 0 corrupt final trees survive the callers' map_constraint_nodes verify-and-discard (no wrong answer). Fix = replace the size-check with a full left/right arborescence check (O(n)). Files: findings.md + to-do.md (T-333), log.md (area-13 round entry). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tor (T-333) The T-327 backstop in impose_one_pass's try_move — postorder.size() == n_internal (commit 6b60f23) — is not equivalent to structural validity. topology_spr's root-child degenerate case can emit a net-zero corruption (one internal node double-referenced +1, one orphaned -1) that lands on exactly n_internal and slips a size check, then feeds a malformed tree to the next reanchor DFS and to reroot_at_tip's parent walk. (The hypothesised std::bad_alloc P1 was refuted — no reachable cycle on either vector — and no malformed tree was ever returned; correctness rested entirely on the callers' verify-and-discard. This makes the guard self-sufficient.) Add structurally_valid() (O(n): child slots in range + left!=right; in-degree 1 for every non-root node / 0 for root; a root DFS visiting every node exactly once; parent[] the exact inverse of left/right). try_move now validates before rebuilding the postorder and reverts on any corruption. Performance: the check is confined to impose_one_pass/try_move, which runs ONLY when constraint= is passed to MaximizeParsimony. TreeState::build_postorder (97 hot-path call sites) is untouched and keeps its > n_internal cap as the bad_alloc backstop, so unconstrained search pays nothing; within a constrained run it is the same O(n) complexity class as the build_postorder already there. Verification: - Exhaustive harness (dev/red-team/heavy-tests/impose_validity/) re-pointed at the real, verbatim-extracted structurally_valid: probe() now asserts it equals the independent full_validity() oracle on every tree, with a g_guard_mismatch gate (exit 3 if they ever diverge — the durable CI signal against reverting to a size-only check). Result: 0 disagreements across all (2n-3)!! rooted binary trees n=4..8 (every net-zero witness the old guard admitted is now rejected). - R integration test added (test-ts-impose-constraint.R "T-333: structural guard survives root-child repair, honours constraint") — the stricter guard neither crashes nor over-rejects valid repairs through the linked engine. - Package builds clean (-Wall -pedantic); full test file green. findings.md row removed (resolved); to-do.md T-333 -> FIXED (pending CI); log.md area-13 round annotated. GHA CI (ASan + full check) pending on push. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fitch_na_score()'s first-uppass tip loop iterates every tip 0..n_tip-1 and dereferences tree.final_[tree.parent[tip] * total_words + ...]. That assumes every tip is attached, but a partial tree can legitimately be scored while some tips are detached: expand_and_reinsert() (prune-reinsert perturbation, T-266) maps the optimised reduced backbone into the full node space and calls score_tree() BEFORE the dropped tips are Wagner-reinserted. Those dropped tips still carry the parent == -1 sentinel from init_wagner_state(), so anc == -1 and the index becomes (-1) * total_words == -total_words, a wild negative subscript (size_t-wrapped). That is out-of-bounds on tree.final_ / tree.prelim. Only the NA path trips this: the EW path (fitch_uppass) and every other pass in fitch_na_score (Pass 1, Pass 2 internals, Pass 3) walk tree.postorder, which contains only attached nodes, so they already ignore detached tips. The NA first-uppass tip loop was the lone place reading tree.parent[] for all tips unconditionally. It aborted under the hardened libstdc++ assertions in the gcc-ASAN CI (run 28681669404, ts-prune-reinsert group; _GLIBCXX_ASSERTIONS operator[] __n < size(), long unsigned int == uint64_t on x86-64). Under a normal build the same read silently touches out-of-bounds heap. Fix: guard the tip loop with `if (anc < 0) continue;`, making the NA uppass honour the same attached-nodes-only contract the EW path already follows. The guard is inert for a complete tree (every tip's parent is >= n_tip), so a normal search is behaviourally identical. Detached tips get their prelim/final recomputed by wagner_incremental_rescore() when they are reinserted, and the accept-gate score_tree() runs on the completed tree, so scoring is unaffected. Regression coverage: the NA and IW score-self-consistency tests in test-ts-prune-reinsert.R (both drive prune-reinsert on the inapplicable Vinther2008 dataset; IW reuses it via fitch_score_ew -> fitch_na_score) exercise this exact path and previously aborted before their assertions ran. Verified locally with a -D_GLIBCXX_ASSERTIONS -O0 -g build: test-ts-prune-reinsert.R now completes 52/52 with no abort (was exit 134), and the ts-na-* / ts-driven groups still pass with no assertion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> (cherry picked from commit 638a6ce)
TS_NA_INCR_AUDIT is an opt-in dev env var (never set by any user-facing code path) that cross-checks the native-NA incremental dirty rescore against full_rescore per TBR candidate. The Rcpp::stop()-based mismatch check is still load-bearing -- it's the sole assertion behind test-ts-na-incremental.R's "TS_NA_INCR_AUDIT cross-check runs clean" test -- so that stays. The na_incr_n/na_incr_ok counters and the REprintf summary they fed existed only to print "N/N candidates byte-matched" once the sweep finished. Because na_incr_ok is only ever incremented immediately after the mismatch check, and any mismatch throws Rcpp::stop() before that increment runs, the ratio can never be anything but N/N -- the message is tautological and was never read by anything (not the test, not a caller). Pure vestige of the incremental dirty-rescore validation work, now that the optimisation is production default; same pattern as the DEBUG_RESCORE/DEBUG_NA_RESCORE/DEBUG_NNI_RESCORE cross-checks removed once validated per T-304. No behavioural change. Verified: clean -O2 rebuild, test-ts-na-incremental.R (91/91) and test-ts-tbr-search.R (28/28) pass.
…y (T-328, T-323) (#261) simplify_patterns() indexed token_states[tip_data-1] with no range check, so an out-of-range tip_data value from a hand-crafted matrix passed to the unexported ts_wagner_tree()/ts_random_wagner_tree()/ts_resample_search()/ ts_parallel_resample()/ts_successive_approx()/ts_simplify_diag() entry points read out of bounds (0 -> before the array; > n_tokens -> past the end, likely a segfault). Add a shared validate_tip_data_values() check at the single Rcpp boundary and its four duplicated call sites (Rcpp::stop on violation), and give ts_simplify_diag its first weight/levels length guards while there. Bundled with the sibling T-323 gap: ts_wagner_tree's addition_order was converted 1-based -> 0-based with no length/range/duplicate check, so a short vector read past its end (reproduced segfault) and an out-of-range or duplicated value corrupted the tree via OOB heap writes. Add validate_addition_order() mirroring the existing weight/levels/min_steps guards. Only reachable via TreeSearch:::, not the public API (AdditionTree()/ MaximizeParsimony() always build valid inputs from a checked phyDat).
…R#264) (#265) Any dataset that collapses to 0 Fitch blocks (total_words == 0) -- not just the all-hierarchy HSJ/xform case PR #264 fixed in wagner_tree/load_tip_states, but also plain equal-weights data where every character is constant or an autapomorphy -- leaves tip_states/prelim/edge_set genuinely empty std::vectors. Code that unconditionally took the address of element 0 of these empty vectors is undefined behaviour: it aborts under hardened libstdc++ assertions/ASan but is silent (out-of-bounds address, never dereferenced if the following block loop is also 0-trip) in a plain release build. Audited every zero-word-reachable site sharing this pattern and guarded each with the same have_words ternary wagner_tree already uses (never bare nullptr+0 arithmetic, which UBSan's pointer-overflow check would still flag): - ts_sector.cpp: compute_from_above_for_sector (called unconditionally on every build_reduced_dataset(), i.e. every sector reduction including the default rasStarts=1 -- the most severe of these, reachable with no non-default control), build_ras_sector (the RAS-restart start-tree builder, reachable via SearchControl(rasStarts>=2)), and build_reduced_dataset_collapsed (reachable via sectorCollapseTarget>0). - ts_prune_reinsert.cpp: build_reduced_dataset and expand_and_reinsert, reachable via pruneReinsertCycles>0 (auto-selected by the "large" preset at nTip>=120). - ts_temper.cpp: stochastic_tbr_phase (annealing), same "large"-preset reachability, needed its own top-level total_words==0 early return matching nni_search/tbr_search/ratchet_search/drift_search's existing style. - ts_tree.cpp: TreeState::save_node_state's prealloc fast path -- a shared primitive under TBR/ratchet/temper, guarded once at the source. - ts_wagner.cpp: wagner_goloboff_scores/wagner_entropy_scores (biased-Wagner tip scoring), also reachable via the "large" preset. Regression tests added to test-ts-sector.R (default control, rasStarts=3, sectorCollapseTarget=6, pruneReinsertCycles=1, annealCycles=1, all through MaximizeParsimony() on a 40-tip all-uninformative EW dataset) and test-ts-wagner.R (ts_wagner_bias_bench directly). Reachability of all 6 sites was confirmed empirically (temporary REprintf probes on each total_words==0 branch, fired under every new test, then removed). Verified the tests are non-tautological, not just reachability-proxy: a negative control reverting build_ras_sector's guard back to the unconditional &rd.data.tip_states[...] reproduces the exact predicted abort (Assertion '__n < this->size()' failed) under a local -D_GLIBCXX_ASSERTIONS -O0 -g build, with a gdb backtrace confirming build_ras_sector (ts_sector.cpp) -> search_sector -> xss_search -> run_single_replicate -> driven_search -> ts_driven_search, i.e. reachable from the public MaximizeParsimony() API. Restoring the guard reruns clean. Full targeted suite (sector, wagner, hsj, xform, anneal, driven, drift, ratchet, tbr, Concordance) passes under the same hardened build with no behavioural regression. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…OX4/NODIRTY) The IW/XPIWE x4 reroot-batch + extract_char_steps dirty-region opts (byte-identical to the scalar path per the 3725746 validation + the dirty-rescore testthat guard) were opt-in (TS_IW_X4/TS_IW_DIRTY, default-off). Flip to default-ON with kill-switches TS_IW_NOX4/TS_IW_NODIRTY, so production IW/XPIWE search runs the ~14-18% faster path by default. All correctness prerequisites already on cpp-search: iw_family gate (fires on XPIWE), the active_mask!=0 nx_cs underflow guard, and the byte-identity test. Test updated to toggle via the kill-switch (else the off-arm would be a no-op under the new default = tautological). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
simplify_patterns precomputed "topology-independent" per-pattern steps via three unsound transforms; the optimal resolution of an ambiguous token is topology-dependent, so they mis-scored (usually over-counted) any matrix with multistate ambiguity: - singleton-removal charged a fixed autapomorphy for a state unambiguous in one tip but also an option inside ambiguous tokens; - redundant-ambiguity removal dropped an ambiguous-only state present in >=2 tips (which can form a cost-free clade on some tree); - verify_uninformative declared a fixed cost from only 4 caterpillar orderings. Gate the transforms to states appearing in <=1 tip, add an unambiguous fast path, and treat full-"?" tips as inert. Affects both CharacterLength and the search scorer. Validated: 80k-eval fuzz vs phangorn clean, suite green. Corrected test refs (test-ts-iw, test-ts-wagner) that were baked under the bug. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A 34-matrix 120-180t reach sweep found reach@96=0.68 climbing to reach@250=0.79, with the hard-matrix subset still rising at 500 (no knee). Raising the cap anytime-dominates (a higher cap only appends later replicates; it never delays an earlier improvement) and easy datasets still stop early on targetHits, so the extra cost falls only on the genuinely hard tail. Scoped to the large strategy; an explicit maxReplicates is always respected (captured before the strategy-scaled default reassigns the formal). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The bespoke `large` preset (T-179) was a cost-cut predating the thorough/auto overhaul: outerCycles=1 (never re-ran sectorial after ratchet), wagnerStarts=1, driftCycles=0, adaptiveStart=FALSE, xss/rss/css 3/2/1. Two fixed-engine MPT-reach sweeps (project175 held out) show thorough's provisioning dominates at large-tree scale: - Long matched wall (1200s, 6 matrices 125-482t): thorough reach 0.61 vs large 0.28; outerCycles=2 is the load-bearing knob (ablation). - Short-budget gate (30/60/120s, 5 matrices 125-199t x 5 seeds): thorough dominates reach at >=60s and ties large+oc2 at 30s; the apparent 30s dip was one seed on one already-solved matrix (tied gap, noise). thorough ran unstarved at 482t/1200s (~187 reps/seed), confirming its heavier provisioning does not rep-starve now that maxReplicates=500 is the large default. Dropped (superseded by thorough's machinery in-sweep): annealing, biased-Wagner start, prune-reinsert NNI polish, tbrMaxHits=1. The per-replicate reach deficit that survives (project5432/4138: 0 hits over 1870 reps) is an engine limit (cross-set sectorial re-solve) that no preset provisioning can close. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lt-OFF)
Add two additive, environment-gated, default-OFF levers to rss_search()
implementing TNT `sectsch` sector-size scaling, aimed at large trees:
(a) nTip-scaled cap: effective max sector size = min(cap, ceil(frac*nTip))
once nTip >= threshold. Env TS_SECT_MAXFRAC / TS_SECT_THRESHOLD (default
88) / TS_SECT_MAXSIZE -- the last raises the absolute ceiling so the
fraction can bind UPWARD on large trees (with the 50/80 preset cap the
min() would otherwise only shrink or be inert).
(b) adaptive growth (TNT increase/selfact/moveon): ramp the sector-size
window up to the cap, M=(T*100)/(selfact*S) selections per band, grow by
increase% per band; moveon early-stop is gated on reaching the cap (and
resets the fail counter per band) so a small moveon cannot leave growth
inert. Env kill-switch TS_SECT_GROW + _INC/_SELFACT/_MOVEON/_START.
Implemented via guarded blocks + equal-valued substitutions (not a duplicated
loop): with no env override and default params the executed statements and the
RNG-draw sequence are byte-identical to the pre-patch path -- verified against
a pristine-HEAD build (identical score/edge/sector-counts for a fixed seed).
Env is read per-call (not static) so the knobs are unit-testable and an A/B can
be driven from a single build. Only rss_search touched (RSS is the sole home
for minsize/maxsize/increase/selfact/moveon); XSS/CSS unchanged.
SectorParams gains the config fields (default-OFF); wiring them through
SearchControl and the `large` preset is a follow-up gated on a matched-wall
Hamilton A/B win.
Tests: tests/testthat/test-ts-sector-scaling.R (off==baseline, kill-switch,
threshold-gating, growth engagement, cap upward-binding, moveon regression
guard); 90 sector-area assertions pass. A/B harness
dev/benchmarks/bench_sector_scaling_ab.R + design
dev/plans/2026-07-08-sector-size-scaling.md.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…#266) * feat(sector): TNT-style in-sector drifting for large sectors (opt-in) Large sectorial sub-problems have more reach but get stuck in their own local optima under plain RAS+TBR. Add TNT `sectsch godrift`/`gocomb`: sectors with >= sectorGoDrift real tips are re-solved by tree-drifting; >= sectorGoComb by combined analysis (sectorCombStarts RAS+drift starts, keep-best). Reuses the existing drift_search; default-OFF (both thresholds 0) so every current search is byte-identical. Anchoring: an unmasked drift on the HTU-anchored reduced sector tree floats the HTU (re-roots the sector vs the rest of the tree) in ~80% of solves, which the root-structure check then reverts to a NON-TBR'd topology -> net-negative. Fixed by threading an optional sector_mask through drift_search -> drift_phase and its internal tbr_search calls (additive, default nullptr; top-level driven_search drift unaffected): search_sector pins the synthetic root, HTU, content root and its two children per RAS start, cutting the HTU-float revert rate ~80% -> ~12%. Residual reverts stay safe via the existing root_ok check. Validated (Hamilton matched-wall A/B, 4 EW sector-resolve datasets x 5 seeds, rasStarts=3 both arms, sectorMaxSize=80): pinned godrift wins-or- ties every dataset with zero regressions for 3-cycle arms (sectorGoDrift 25-40, sectorDriftCycles=3). Shipped OPT-IN only; the recipe-default flip is gated on a separate preset-level (rasStarts- matched) A/B. The gocomb fuse recombination sub-step is deferred (sectorFuseRounds reserved/unused). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(sector): implement the gocomb fuse recombination sub-step (default-off) (#267) * feat(sector): implement the gocomb fuse recombination sub-step Completes TNT combined analysis for large sectors: after the sectorCombStarts RAS+drift starts, recombine them with tree_fuse (best start as recipient, the others as donors) and keep the fused tree only if it beats every start. Previously gocomb was RAS+drift keep-best; the fuse step was deferred because tree_fuse re-roots the reduced tree at tip 0, dropping the (HTU, sr_mapped) synthetic-root layout reinsert_sector needs. Re-anchoring: parsimony is rooting-invariant, so reanchor_fused_at_htu simply re-roots the fused tree at the HTU pseudo-tip. The HTU represents the rest of the tree at the sector boundary and stays adjacent to the content root, so its sibling after re-rooting is the whole non-HTU clade -- exactly what reinsert_sector grafts back, with the content clade still rooted at sr_mapped. The fused tree is accepted only through the SAME root_ok gate + reinsert path as the drift starts (no reduced-score shortcut); if fuse ever floated the HTU into the interior -- the move the drift path already reverts -- root_ok fails and the best RAS+drift start stands, so a fused tree is reinserted only when its id mapping is already valid. Default-OFF (needs sectorGoComb > 0); env TS_SECT_FUSE=0 disables only the fuse step (drift-only comb) to isolate its marginal contribution in an A/B. Reuses the existing tree_fuse and ts_tbr's reroot_at_tip; no new scoring code. Local correctness gate (Zanol2014 74t, Giles2015 78t -- the wps>=2 regime): fuse fires and wins, and every reinserted tree's C++ score equals an independent by-label TreeLength. An initial permutation-remap for the HTU-float case was implemented, found empirically unreachable (the content root always keeps id sr_mapped), and removed to avoid shipping untested pointer surgery. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(sector): matched-wall harness isolating the gocomb-fuse margin Adds dev/benchmarks/sector_fuse_ab.R: within each combStarts level the fuse and drift-only arms differ only by TS_SECT_FUSE, so any delta is fuse alone. Records the pre-committed decision rule (beat drift-only -> opt-in, else stay default-off). This is the harness behind the null marginal result. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…odrift) Flips the godrift mechanism from #266 into a `thorough` default after two preset-level matched-wall A/Bs (2026-07-08, arrays 17836031 then 17836637): rasStarts=3 + sectorGoDrift=25 + sectorDriftCycles=3 beats stock thorough on every dataset class with no regression -- mid-size sector-resolve -1.5, general-pool (68-88t) -0.4, large training matrices (131-205t) -2.7, with the rep-starved 205t project3763 -8 steps and the hard floor project4138 reaching its optimum. intensive/large inherit via alias (confirmed positive/neutral on large trees, never a regression). Details behind the numbers: - sectorGoDrift=25 is calibrated to thorough's sector geometry: xss/cssPartitions=6 make XSS/CSS sectors ~12-15 tips, so drift engages via RSS large-clade picks; the standalone-tuned 40 is near-inert under this preset. - rasStarts>=2 is required (drift's retention channel is inert at 1) and coupled: rasStarts=3 ALONE (no drift) regresses -- it triples every sector-solve for fewer replicates -- but the drift more than redeems the cost. rasStarts=2+drift ties 3+drift at matched wall, so 3 (marginally best, round-1-validated) is kept. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The {-,X} ambiguity TreeLength cases in test-length_range.R post-dated the
strip-to-inapplicable behaviour and asserted the old (buggy) scores, so they
broke after 53ffb05 made {-,X} applicable-preferred (De Laet). char1Dat was a
byte-for-byte duplicate of dat_distinct in test-ts-na-ambig.R.
Per tests/testing-strategy.md, engine-internal correctness is Tier-2's job
(test-ts-*.R, skip_on_cran); test-length_range.R is a Tier-1 R-level API file.
Remove the misplaced block from length_range and relocate the one genuinely
distinct arrangement (neighbour-aligned gradient, length 2) into na-ambig.
Tradeoff: this drops the incidental CRAN smoke of {-,X} scoring. Intentional
and doc-sanctioned -- ambiguity engine correctness is Tier-2, run in CI
(NOT_CRAN=true) but deliberately skipped on CRAN.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Score MorphyLength via native kernel for inapplicable data (Stage 1 of MorphyLib removal)
MorphyLength()/MorphyTreeLength() route the default inapplicable gap mode
through the native Fitch kernel (FastCharacterLength) instead of vendored
MorphyLib. Fixes a known MorphyLib bug scoring `{1-}` ambiguous-with-
inapplicable tokens (Lobo pattern 93: native 2 vs MorphyLib 3), aligning the
legacy custom-search scorers (TreeSearch, Ratchet, Jackknife, custom.Rmd)
with the already-correct MaximizeParsimony.
- PhyDat2Morphy()/SingleCharMorphy() attach precomputed native scoring data
(.NativeMorphyData/.SingleCharNative); the Morphy pointer is retained so
Morphy()'s C-loop and introspection are untouched.
- Rare ambiguous/extra gap modes keep the MorphyLib path for now.
- Not aiming for MorphyLib-equivalence: native scores are the correct v2.0
behaviour. Verified MorphyTreeLength == TreeLength across Lobo + CL
datasets and per character via SingleCharMorphy.
Stage 1 of dropping MorphyLib entirely from 2.0; later stages migrate
Morphy()'s preorder_morphy/morphy_iw/morphy_profile loop and delete the
vendored sources.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Document staged plan for dropping MorphyLib entirely (Stages 2-3)
Captures the migration of Morphy()'s preorder_morphy/morphy_iw/morphy_profile
loop and the ambiguous/extra gap modes to native (Stage 2), and the deletion of
vendored MorphyLib sources/bindings/introspection (Stage 3). Flags the Morphy()
approach (swap closures vs redirect to MaximizeParsimony) as needing maintainer
review, since both change superseded-path behaviour.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Remove Morphy() entirely (Stage 2 of dropping MorphyLib)
Morphy() was the redundant MorphyLib R-loop search (EW/IW/profile + ratchet +
constraints), never released on CRAN (not in 1.8.0), sitting between native
MaximizeParsimony() and the custom-criteria TreeSearch()/Ratchet(). Everything
it did is covered natively, so it is deleted rather than migrated.
- Delete Morphy() + its private helpers (.EWScore/.IWScore/.ProfileScore/
.TBRSearch/.CombineResults/.ReplaceResults/.Time/.DateTime) from R/Morphy.R.
- Replace MaximizeParsimony()'s legacy-parameter delegation (do.call(Morphy,..))
with an informative error.
- Drop export(Morphy); delete man/Morphy.Rd + man/dot-CombineResults.Rd; fix
dangling [Morphy()] doc links (WhenFirstHit, EasyTrees, MaximizeParsimony).
- Delete test-Morphy.R (+ snapshots); drop Morphy() calls from test-CustomSearch.
Leaves morphy_iw/morphy_profile dead (Morphy() was their only caller), to be
removed in Stage 3. Full suite green: FAIL 0, PASS 11201.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Remove vendored MorphyLib; native functional resampling
Drop the vendored Morphy Phylogenetic Library entirely. All parsimony
scoring now runs through the native C++ kernel, which implements the
Brazeau-Guillerme-Smith inapplicable algorithm correctly (including the
`{1-}` token MorphyLib mis-scored). The native engine's own Wagner
builder (ts_wagner.cpp) is independent of MorphyLib, so the library
deletes wholesale - no MPLstate extraction needed.
Custom-search resampling (Jackknife/MorphyBootstrap) now resamples
character weights functionally and scores them with the native kernel,
replacing the in-place mpl_set_charac_weight mutation that the Stage-1
native MorphyLength path silently ignored (a latent regression). The
custom TreeScorer/EdgeSwapper API is unchanged; a new regression test
asserts resampled weights affect the score.
- PhyDat2Morphy/SingleCharMorphy native-only; UnloadMorphy a no-op;
RandomTreeScore native (kept RandomMorphyTree + pure C RANDOM_TREE)
- Removed mpl_* bindings, MorphyWeights/SetMorphyWeights/GapHandler/
MorphyErrorCheck/summary.morphyPtr, GetMorphyLength/C_MorphyLength,
MORPHYLENGTH/RANDOM_TREE_SCORE, morphy_iw/morphy_profile/preorder_morphy
- Deleted 17 vendored MorphyLib C/C++ sources; trimmed TreeSearch-init.c
and build_postorder.h; regenerated RcppExports/NAMESPACE/man
Only the default `inapplicable` gap treatment is supported natively.
Full suite green; the only failures are pre-existing WideSample/MaxMin
API drift, unrelated to this change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Native scoring layer: purpose-based names + concavity (best of #216)
Synthesise the best of PR #251 (this branch) and PR #216 (T-200, Agent B),
which independently decoupled the R-loop search from MorphyLib.
Adopted from #216:
- Efficient native scorer: PrepareData() pre-extracts the scoring matrices
once and EdgeListScore() calls ts_fitch_score() directly, instead of
rebuilding a phylo and re-extracting tip data on every score.
- `concavity` parameter on TreeSearch()/Ratchet()/Jackknife(): Inf = equal
weights (default), finite = implied weights, "profile" = profile parsimony.
Custom InitializeData hooks still receive the dataset unchanged.
- custom.Rmd implied-weights section rewritten from ~150 lines of hand-rolled
scaffolding to a few lines using `concavity` directly.
Kept from #251 (and fixing #216's staleness):
- MorphyLib stays fully deleted (#216 retained it as a fallback).
- Score weights use .ScaleWeight() (fractional-weight aware) and Inf for the
equal-weights concavity, matching current TreeLength(); #216 used
as.integer(weight) and -1.0, predating those changes.
- Functional resampling + a regression test (doubling weights doubles score).
Naming: the layer is renamed to purpose-based names rather than #216's
`*Native*` prefix, because once MorphyLib is gone "Native" no longer contrasts
with anything. PhyDat2Morphy->PrepareData, UnloadMorphy->ReleaseData,
is.morphyPtr->is.ParsimonyData, SingleCharMorphy->SingleCharData,
MorphyLength->EdgeListScore, MorphyTreeLength->TreeScore,
MorphyBootstrap->BootstrapTree, RandomMorphyTree->RandomPostorderTree.
The old names remain as .Deprecated() aliases.
TreeScore() == TreeLength() verified across equal weights, implied weights and
profile parsimony; full suite green (only the pre-existing WideSample/MaxMin
API-drift failures remain, unrelated to this change).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* DESCRIPTION: drop MorphyLib copyright; recast BGS contributors as ctb
MorphyLib C/C++ code was fully removed in 33dc949; the Copyright field
is now factually wrong. Drop it. Brazeau and Guillerme contributed the
BGS inapplicable-data algorithm, not a vendored library, so both are
recredited as ctb with an accurate comment.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* DESCRIPTION: add Guillerme ORCID
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* Regenerate docs after merging cpp-search
Sync roxygen cross-links following the rebase onto current cpp-search
(picks up upstream's BigCamelCase hierarchy-helper renames alongside
this branch's own renames).
* Deprecate InitializeData/CleanUpData; move concavity onto PrepareData()
With MorphyLib gone, neither hook does anything a user can't do just as
well themselves: InitializeData/CleanUpData were called exactly once,
single-threaded, to build/free a C pointer that no longer exists (native
ParsimonyData is plain, GC'd R data). TreeSearch()/Ratchet()/Jackknife()
now pass `dataset` straight through to TreeScorer; callers prepare it
themselves (typically via PrepareData()) before calling. Supplying the
old hooks still works but warns via .Deprecated().
concavity moves off these three functions onto PrepareData(), since it
belongs to the dataset, not the search call -- the default TreeScorer,
EdgeListScore(), reads it from the prepared object directly. Confirmed
empirically that BootstrapTree()'s reweighting-only resampling already
handles IW correctly, including under mostly-zero pattern weights.
Rewrites the custom.Rmd implied-weights section, which no longer needed
any of the removed machinery, to instead show how to hand-write a scorer
of comparable complexity (MinimumLength()/CharacterLength()-based, with
concavity captured by closure) as a template for optimality criteria
TreeSearch doesn't provide natively.
* Fix custom.Rmd: IWScore's minLength was recomputed on every call
minLength = MinimumLength(dataset, compress = TRUE) as a default argument
is a lazy promise evaluated fresh each call, since nothing threads
minLength= through the search loop -- defeating the "compute once,
cache" example it was meant to demonstrate. Drop it as a formal
parameter; the function body now closes over the pre-computed outer
`minLength` instead (a cheap lookup, not a recomputation). Verified the
closure is actually reused across all evaluations in a search, and that
scores still match the native implementation exactly.
* Revise IW examples
* Rewrite
* Missing @imports
* importFrom TreeTools AddUnconstrained
* Split out Morphy.R
& rm dud "deprecated" new MaxPars2
* MaximizeParsimony2 removal
* Delete Sectorial.R
Doesn't benefit from computational time-saving without a custom-written scorer.
Unlikely to escape from optima any better than Ratchet()
Hence unclear what niche this would fill. Verdict = retire
* Update Collate per deleted files
* Rm obselete tests
* Delete run_tests.R
* Spellings
* -@
* syntax error @Vinh2010
* Document Resample()'s live args; deprecate its dead ones
Resample.Rd's undocumented-argument NOTE traced to two separate causes.
Resample() used to share Morphy()'s roxygen block via `@rdname Morphy`,
which was deleted along with Morphy() itself (d324ae1); that accounts
for dataset/tree/ratchIter/tbrIter/concavity/constraint, which are all
still live and just needed honest new docs describing what they map to
in the native search engine (ratchIter/tbrIter in particular have been
silently repurposed from their old Morphy()-era meaning).
Separately, finalIter/maxHits/tolerance/verbosity have done nothing
since Resample() was first rewritten around the native engine -- they
were never wired into ts_resample_search/ts_parallel_resample, which
have no equivalent controls (no progress-reporting hook exists at all).
Rather than write misleading docs for dead parameters on a
soon-to-be-frozen API, deprecate them (default NULL; a warning fires
only if explicitly supplied), matching the InitializeData/CleanUpData
deprecation already applied elsewhere in this PR. Drop the corresponding
now-inert `verbosity = 0` from the tree-search.Rmd example.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Fix R-CMD-check 6h timeout: gate WideSample()'s solver-tier demo behind interactive()
The R 4.1 and R-devel legs were hanging for the full 6h GHA limit at
"checking examples with --run-donttest", while release (sense-check,
windows) passed in minutes. Comparing job logs pinned the difference to
how the 'highs' package builds: on those two legs it compiles its bundled
HiGHS solver from source via cmake (no RSPM binary exists for an archived
old version or for R-devel), whereas the passing legs use a prebuilt
binary. WideSample.Rd's donttest example exercised the highs-backed exact
solver (both explicitly via effort = 4 and implicitly via effort = NULL's
auto tier-selection), and a from-source HiGHS build is apparently slow
enough to blow through the documented 60s maxSeconds budget by hours.
Since MaxMin/highs are Suggests-only and the DropAdd/Grasp/exact tiers'
runtime is inherently environment-dependent (and, per direct testing
against both the CI-pinned and current MaxMin HEAD, its calling
convention is also actively in flux upstream), move every example beyond
the deterministic, matrix-free FarFirst tier behind `if (interactive())`,
matching the existing convention for expensive optional demos (e.g.
Morphy.Rd). Only the always-safe FarFirst call now runs under automated
checks.
* Document Resample() via @inheritParams MaximizeParsimony; fix nThreads=0
Resample() is a MaximizeParsimony() relative, not a TreeSearch()-family
function, so its shared params (dataset, concavity, constraint, hierarchy,
inapplicable, hsj_alpha, extended_iw, xpiwe_r, xpiwe_max_f) should inherit
MaximizeParsimony()'s canonical descriptions instead of restating them,
keeping the two docs from drifting apart. Params where Resample()'s
behaviour genuinely differs (tree: fallback not warm-start; the deprecated
finalIter/maxHits/tolerance/verbosity quartet) keep explicit overrides,
which roxygen2's @inheritParams respects.
While aligning nThreads's documentation with MaximizeParsimony(), found
Resample() silently clamped nThreads to a minimum of 1, defeating the
0 = auto-detect behaviour that MaximizeParsimony() documents and that the
underlying ts::parallel_resample() engine already implements. Fixed to
pass nThreads through unclamped, matching MaximizeParsimony().
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Support inapplicable = "missing" in Resample()
Resample()'s match.arg only accepted "bgs"/"hsj"/"xform", so the
pure-Fitch mode already demonstrated in Resample()'s own @examples
(inapp = "miss") errored. Recode gaps as missing via .GapsAsMissing()
and fall through the bgs path, matching MaximizeParsimony()'s
treatment exactly.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* rm cruft
* Redoc Jackknife
* Redoc jackknife
* Update blurb
* -Morphy
* Quiet tests
* Message suppression
* rm unneeded comment
* No morphy → no Postorder needed
* -morphy refs
* Quieten
* Rm Morphy refs
* Rm stale Morphy references
* Faster example
(!)
* Resample dox
* -stale RoxygenNote
* dup param
* Delete bench-iw.R
Obselete
* Update MorphyLib migration doc: mark plan complete, flag divergences
Trims completed tables/phases now that Tiers 0-4 and Phases A/C/D are all
done on this branch, and records where the actual outcome diverged from
what was planned: Morphy() was deleted rather than migrated, the rare
gap treatments were dropped rather than reproduced, and
InitializeData/CleanUpData were deprecated rather than merely re-pointed
at native defaults. Notes the one thing still open: merging #251 into
cpp-search, which still carries the full vendored library.
* Delete morphy-migration.md
* Compact
* obselete
* Delete 2026-06-15-morphylib-removal-stages.md
* Rm redundant / obsolete
* Update test-ts-na-ambig.R
* -Simd refs; rm empty test
* Format
* Rm morphy refs
* clarity
* Redoc
* Delete debug_ratchet.R
rm stray debug file
* Standalone `Resample()`
* Restore example setup
* Update Resample.Rd
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Audit of the documentation surface touched by the 2.0.0 release. Stale/incorrect claims corrected: - Preset docs described the retired bespoke `large` preset (biased Wagner, annealing, reduced cycles); `large` is now an alias of `thorough` with maxReplicates=500. Fixed in MaximizeParsimony roxygen, the SearchControl comment, and the search-algorithm / tree-search vignettes. - `default` preset documented as 12 ratchet cycles; it is 6. - Vignette preset claims about adaptiveStart, ratchetTaper, annealCycles and consensusStableReps contradicted the actual SearchControl defaults / preset values. - MaximizeParsimony profile-parsimony docstring claimed binary-only (max 2 states); multi-state is supported (Carter 1990 / Maddison & Slatkin 1991 / Monte Carlo). ConcordantInformation carried the same stale two-state restriction. - PrepareDataProfile: `bootstrap` attribute documented a `split.sizes` entry it never writes; "with a warning" overstated (only the inapplicable-token simplification emits a message). - WideSample `@return` said "uniform sample"; it returns a Max-Min diverse subset. - fractional-weight default documented as 1000L; it is 1260L (NEWS.md). - Implied-weight fit formula rendered as `k / e + k`; corrected to `k / (e + k)` (propagates to inheriting man pages). LLM artefacts removed: - The "reflects island structure, not a cap" framing in the Completeness section (the "256 is not hard-coded" aside was already removed on the branch tip by #251). - NEWS.md: the "common complaint" defensive aside, the empty "Other improvements" section, and a duplicate PaintCharacters bullet. man/*.Rd regenerated with roxygen2. Not addressed here (left for the maintainer): the "To integrate into 2.0.0 notes" scaffold in NEWS.md now also holds #251's breaking-change bullets, and the "renamed to Morphy()" line is stale since #251 removed Morphy() — final integration of that section is #251's to make. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…ness
Recovered from a GitHub Desktop auto-stash (June) applied via `git stash pop`.
- dev/plans: gap-framing, lazy-precompute-incremental-length,
lever3-incremental-edgeset design specs (siblings of tracked plans)
- dev/red-team/reviews/cpp-search-sect-colreduce: Block-verdict review of
the per-sector column-axis reduction (F1 heap-overflow) + 2 repros
- dev/red-team/{focus-areas,log}: process-record updates (both area-13
entries preserved, append-only newest-first)
- dev/benchmarks: tbr_throughput_race harness + wall instrumentation in
tbr_shared_start_lib
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
1 participant
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.