Skip to content

T-385: report the x-transformation score of the tree actually returned - #278

Open
ms609 wants to merge 7 commits into
cpp-searchfrom
claude/t385-xform-report-agreement
Open

T-385: report the x-transformation score of the tree actually returned#278
ms609 wants to merge 7 commits into
cpp-searchfrom
claude/t385-xform-report-agreement

Conversation

@ms609

@ms609 ms609 commented Jul 31, 2026

Copy link
Copy Markdown
Owner

Implements the already-decided Option 3 of dev/plans/2026-07-29-t374b-xform-rooting-policy.md. That decision's three merged commits (7a18a4b7 et al.) turned out to be docs only — no code implemented "make MaximizeParsimony's reported score and TreeLength() agree on one rooting".

Stacks on #277. Branched on top of claude/ci-easy-fixes so sense-check could pass and CI give a meaningful signal; merge #277 first. Only the last four commits here are T-385.

⚠️ Reviewer-critical: reported XFORM scores change

They rise, and will never fall — measured 178 → 183 on the repro. The new value is the length of the tree you are actually handed; the old one was the length at a rooting discarded during search. Documented in NEWS, ?MaximizeParsimony, ?RecodeHierarchy and vignettes/search-algorithm.Rmd. Only inapplicable = "xform" is affected.

The defect

XFORM's step matrix is asymmetric (gain nSec + 1 against loss 1), so a tree's length depends on where it is rooted — unlike the symmetric criteria. MaximizeParsimony reported result$best_score, recorded mid-search at whatever rooting the replicate held, while ts_collapse_pool hands every tree back re-rooted on tip 0.

Reproduced against tip a8fbba84 before writing any fix (36 tips / 6 blocks / nSec 2):

quantity pre-fix post-fix
reported attr(res, "score") 178 183
TreeLength() of all 32 returned trees 183 183
same topology over 8 rootings 178–183 (spread 5) 183 (spread 0)

The reported number was not wrong — 178 is attained by 2 of those 8 rootings. It was a value at a rooting the user never receives.

The fix

Canonicalise on the dataset's first taxon at both boundaries:

  • TreeLength() — in both methods. The multiPhylo path previously rooted only trees that arrived unrooted (tree_length.R:298), so an already-rooted tree kept its own rooting. Rooted by name with tips re-aligned afterwards, never by an R reroot on the edge matrix (that permutes tips and mis-scores against tip_data).
  • MaximizeParsimony() — rescore the returned pool through that same path, |pool| evaluations.

Not done, deliberately: min-over-rootings reporting. The plan calls it the better variant, but the deciding objection is cost — naively (2n-3)× on the Sankoff term, so a 4000-tip pool of 100 trees needs ~800k boundary evaluations. Affordably it needs an all-rootings up-down DP, which is Option 4's own piece of work.

HSJ reporting is untouched, gated on XFORM alone with the reason in the code: there rooting-invariance is required by the method (two-state DP, T-374's open half), so canonicalising would convert a wrong objective into a stably-wrong one.

Scope boundary — and a residue this surfaced

Pool membership is still decided on search-time scores taken at differing rootings, so returned trees need not share the canonical length. That is T-374's open residue (fixing it moves n_topologies semantics and interacts with collapse, unanalysed by the decision document) — but it is no longer silent: MaximizeParsimony() now warns and reports the smallest.

That warning immediately found a live instance in the existing suite: all-hierarchy xform data returns a pool spanning 7 to 9. It independently corroborates T-374's original "4 of 6 trees do not share a score", and is now pinned as an expect_warning so it reads as a documented expectation rather than noise — and so that if it ever stops firing, someone notices.

Tests

Two regression tests, both verified to fail against a pre-fix build in a throwaway worktree (spread 2 not 0; reported 46 against min 48) and pass after:

  • The deterministic one carries an anti-vacuity guard: it first asserts, via ts_sankoff_test — untouched by this fix — that the raw kernel really is rooting-sensitive on its 8-tip input, so it cannot pass by scoring rooting-insensitive data.
  • The end-to-end one asserts the actual contract (reported == min of the returned trees' canonical lengths), not the stronger "every tree shares it", which holds for its data but is not what the code promises.

Also corrects the three false root-invariance comments the plan lists (ts_tbr.cpp:123 and :3152, ts_rcpp.cpp's collapse reroot, MaximizeParsimony.R's collapse note).

Validation

All 8 jobs green on run 30662279563. (ubuntu-24.04 (4.1) failed once at "Set up R dependencies" with the Bioconductor 3.14 repos unreachable — Check package was skipped, so no code was tested; green on re-run.)

Locally, NOT_CRAN=true across xform, tree_length, hsj, t330, recode-hierarchy, resample-hierarchy, t306, prune-reinsert — 0 failures. spell_check_package() clean; check_man() clean.

🤖 Generated with Claude Code

ms609 and others added 7 commits July 31, 2026 09:09
Tests were already green (FAIL 0 across 11612); the coverage wave was skipped
because sense-check reported 1 ERROR, 1 WARNING and 2 NOTEs, all from the recent
`effort` work.  This clears the two that gate the build, plus one long-standing
NOTE that was generating repeated man-page churn.

ERROR (spelling): `Zanol`.  `inst/WORDLIST` already carried `ZANOL`, from
`R/data.R`'s all-caps citation, but not the mixed-case form that reached prose.

WARNING (undocumented argument): `.rung`, added to `MaximizeParsimony()` without
a `@param`.  R CMD check requires every argument in a `\usage` section to be
documented even when it is deliberately internal, so it is now documented AS
internal -- what it pins, that `"none"` means no preset at all, that `NULL` is the
ordinary path, and that callers should prefer `effort`.  This is the check
AGENTS.md names for a signature change (`devtools::check_man()`); running it would
have caught this before the push.

NOTE (empty \references): `expected_mi`'s roxygen carried
`@references \insertAllCited{}` while citing nothing, so Rd generation emitted an
empty `\references` section.  Dropped at source in `src/expected_mi.cpp`.  This
also stops `man/expected_mi.Rd` re-churning on every unrelated roxygen run.

NOT touched, and worth knowing:
- "Version contains large components (2.0.0.9999)" is self-inflicted by CI, not by
  the repo.  DESCRIPTION says `2.0.0`; the workflow's own "Temporarily bump package
  version" step (.github/workflows/R-CMD-check.yml:77) appends `.9999` whenever the
  version has exactly two dots.  Fixing it means changing that step, which is a
  release-process decision rather than a docs fix.
- The `Remotes` field and the `MaxMin` suggestion are flagged by the CRAN-incoming
  check but are deliberate for a development package.
- `man/SiteConcordance.Rd`'s DOI 10.1201/9780367823412-8 now 404s upstream.
None of these three is a WARNING or an ERROR, so none of them gates the build.

Verified: `spell_check_package()` clean; `tools::checkDocFiles()` reports no
undocumented or mismatched arguments; the `man/expected_mi.Rd` diff is exactly the
three lines of the empty section and nothing else.

Co-Authored-By: Claude Opus 5 <[email protected]>
…hine's speed

The coverage wave ran for the first time and immediately failed on my own parallel
stopPatience test: FAIL 1 / PASS 11627 under `covr::codecov()`, against FAIL 0 /
PASS 11628 in the same commit's ordinary check.  So the test was not wrong about
the code -- it was wrong about the environment.

Mechanism, and it is the one I documented in the very commit that added the test
(998891e): on the parallel path the dry spell is counted over replicates completed
into the shared pool and evaluated only when the coordinating thread polls, every
200 ms.  covr instrumentation makes each replicate far slower, so far fewer land
per poll interval and `stopPatience = 3` fires genuinely earlier in the search --
13.063 against 12.988.

`expect_equal(short score, full score)` was therefore asserting a coincidence of
timing, not a property of the rule.  Stopping sooner is *allowed* to cost score;
that is the entire trade-off `stopPatience` exists to offer, and it is what the
shipped preset measurements quantify.  Asserting equality contradicted the
feature's own documentation.

What the rule does guarantee is already asserted and untouched: the search stops
before `maxReplicates`, and `perturb_stop` is set.  The score check is now only
that it is finite and not grossly worse, which holds at any speed.

Third time this session a test of mine encoded an environment assumption -- after
`as.Splits`/`%in%` dispatch and the ARM64 ordering chase.  The pattern: an
assertion that passes because of how fast, how attached, or how ordered the local
environment happens to be, rather than because of what the code promises.

Co-Authored-By: Claude Opus 5 <[email protected]>
Pre-fix reproduction against cpp-search tip a8fbba8, written before any fix so
the failure is on record independently of the change that addresses it.  The
finding was recorded today but the tip has moved since (T-391 landed), and
[[redteam-verify-against-current-tip]] says to re-verify rather than assume.

All three symptoms reproduce on a 36-tip / 6-block / nSec=2 synthetic dataset
(seed 1, search seed 11, maxReplicates 4):

  reported attr(res, "score")           178
  TreeLength(res[[i]], ...) as returned 183  for all 32 trees   -> gap 5
  same topology, 8 rootings             178..183, spread 5      (bound Sum nSec = 12)
  32 MPTs at a common tip-1 rooting     183 and 182             -> 2 distinct

Note the reported 178 IS attained, by 2 of the 8 sampled rootings: the engine is
not computing a wrong number, it is recording a score at one rooting and
returning a topology rooted elsewhere (ts_collapse_pool's tip-0
canonicalisation being the trigger site named in T-374).

The script reports rather than asserts, and exits 1 while the gap is open, so it
doubles as the acceptance check for the fix.
Implements the ALREADY-DECIDED Option 3 of
dev/plans/2026-07-29-t374b-xform-rooting-policy.md, whose three commits on
cpp-search (7a18a4b et al.) turned out to be docs only -- no code implemented
"make MaximizeParsimony's reported score and TreeLength() agree on one rooting".

## The defect

XFORM's step matrix is asymmetric (gain = nSec + 1 against loss = 1), so a
tree's length depends on where it is rooted -- unlike the symmetric criteria.
MaximizeParsimony reported `result$best_score`, recorded mid-search at whatever
rooting the replicate held, while ts_collapse_pool hands every tree back
re-rooted on tip 0.  So the reported number was not the length of the tree
returned, and re-rooting a returned tree changed it again.

Reproduced on tip a8fbba8 (36 tips / 6 blocks / nSec 2), pre-fix:

  reported 178  |  TreeLength(returned) 183 on all 32 trees  |  gap 5
  same topology over 8 rootings: 178..183, spread 5 (bound Sum nSec = 12)

The reported number was not WRONG -- 178 is attained by 2 of those 8 rootings --
it was a value at a rooting the user never receives.

## The fix

Canonicalise at both boundaries, on the dataset's first taxon (the tip-0 rooting
ts_collapse_pool already imposes):

- TreeLength(): root before scoring, in BOTH the single-tree and multiPhylo
  methods.  The multiPhylo path previously rooted only trees that arrived
  unrooted, so an already-rooted tree kept its own rooting.  Rooted by NAME with
  tips re-aligned afterwards, never by an R reroot on the edge matrix
  (na-validation-alignment-gotcha).
- MaximizeParsimony(): rescore the returned pool through that same path and
  report the result -- |pool| evaluations.

Post-fix the acceptance script is clean: gap 0, spread 0 across 8 rootings.
Scores may therefore differ from previous versions and will not decrease; the
value is now the length of the tree in hand, an upper bound on the rooting-free
minimum exceeding it by at most Sum nSec (attained by 87-98% of rootings).

Deliberately NOT done: min-over-rootings reporting.  It is what the plan calls
the better variant, but it reports a quantity the search never compared and costs
(2n-3)x on the Sankoff term, so it stays Option 4.  HSJ reporting is also
untouched -- there rooting-invariance is REQUIRED by the method (two-state DP,
T-374's open half), so canonicalising would convert a wrong objective into a
stably-wrong one.  Gated on XFORM alone, with that reason in the code.

## Scope boundary, and a warning where it bites

Pool membership is still decided on search-time scores taken at differing
rootings, so the returned trees need not share the canonical length.  That is
T-374's open residue, out of scope here -- but no longer silent:
MaximizeParsimony now warns and reports the smallest.  The warning immediately
found a live instance in the existing suite (all-hierarchy data spans 7 to 9),
which independently corroborates the earlier session's "4 of 6 trees do not share
a score" observation.

## Tests

Two regression tests, both verified to FAIL against the pre-fix tip built in a
throwaway worktree (spread 2 not 0; reported 46 against min 48) and pass after:

- The deterministic one carries its own anti-vacuity guard: it first ASSERTS via
  ts_sankoff_test -- untouched by this fix -- that the raw kernel really is
  rooting-sensitive on its 8-tip input, so it cannot pass by scoring
  rooting-insensitive data.
- The end-to-end one asserts the actual contract (reported == min of the returned
  trees' canonical lengths), not the stronger "every tree shares it", which holds
  for its data but is not what the code promises.

Also fixes the three false root-invariance comments the plan lists
(ts_tbr.cpp:123 and :3152, ts_rcpp.cpp's collapse reroot, MaximizeParsimony.R's
collapse note), and documents the behaviour in ?MaximizeParsimony,
?RecodeHierarchy, NEWS.md and vignettes/search-algorithm.Rmd.

Suite green: xform, tree_length, hsj, t330, recode-hierarchy,
resample-hierarchy, t306, prune-reinsert -- 0 failures.
… rename

Not part of T-385 -- surfaced by running check_man() for it.

419168d changed the roxygen example from "one notch less / effort = -1" to
"one notch more / effort = 1L" but man/MaximizeParsimony.Rd was never
regenerated, so the shipped example contradicted its own source. Regenerated
from the roxygen block, which is authoritative.
The acceptance script carried its own copy of MakeXformData() identical to the
one the rooting diagnostic sources, so the two could silently drift.  Sourced
from the shared file instead.  Acceptance script re-verified: exit 0, gap 0,
spread 0 (checked WITHOUT a pipe -- the earlier 'EXIT: 0' was tail's status).
Three follow-ups, no behaviour change.

- test-ts-xform.R's all-hierarchy test now expect_warning()s the "do not share a
  length" warning instead of emitting it into an otherwise-green suite.  That
  matrix provably triggers T-374's residue (pool spans 7 to 9), so the warning is
  an expectation, not noise -- and if it ever stops firing, the residue has been
  fixed or masked and this test says so.  Assign INSIDE expect_warning(): it
  returns the condition, not the expression's value, so `res <- expect_warning(...)`
  silently bound the warning object and broke the two assertions below it.

- Both diagnostic scripts source() a path relative to the package root, so they
  only work when invoked from there.  They now stop with that message instead of
  failing obscurely -- the same relative-path trap that made five tree_length
  tests "fail" earlier today when test_dir() changed the working directory out
  from under a relative lib.loc.

- Recorded the real reason min-over-rootings reporting is declined.  "Reports a
  quantity the search never compared" is the weaker half; the deciding objection
  is cost -- naively (2n-3)x on the Sankoff term, so a 4000-tip pool of 100 trees
  needs ~800k boundary evaluations.  Affordably it needs an all-rootings up-down
  DP, which is Option 4's own piece of work.
ms609 added a commit that referenced this pull request Jul 31, 2026
All 8 CI jobs green on run 30662279563. The one ubuntu-24.04 (4.1) failure was
transient -- it died in 'Set up R dependencies' with the Bioconductor 3.14 repos
unreachable, so 'Check package' was skipped and no code was ever tested; green on
re-run of that job alone.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant