fix(hsj): make the HSJ score rooting-invariant (T-374, HSJ half) - #281
Merged
Conversation
…h (T-374) A secondary character exists only where its controlling primary is present, so at a tip whose primary can code the structure absent its "-" is not a state the character takes -- it is the statement that the character does not apply there. fitch_label_char() nonetheless admitted "-" as an ordinary concrete state, letting the uppass propagate it inwards and resolve a node in the middle of the PRESENT region to it, where it is disjoint from every present neighbour in every secondary at once. score_hierarchy_block() then charged that branch d = m -- the full alpha -- for a node that by construction has no inapplicable secondaries. The over-charge is wrong under any rooting: the paper's d is "the number of nonmatching secondary characters" (Hopkins & St John 2021, p.5) among the characters that APPLY. Because whether it fired depended on the DELTRAN direction, it was also the dominant source of T-374's rooting-dependence. Secondaries are now wildcarded at tips whose primary token can mean absent, keyed off the PRIMARY rather than off the secondary's own token: a "-" secondary at a tip whose primary is unambiguously present is contradictory data that ValidateHierarchy rejects upstream, and the kernel keeps scoring it as a concrete state rather than silently reinterpreting it. Measured over every edge-rooting of 30 random 9-tip topologies, alpha = 1: regime before after all-present 0/30 0/30 (already exact -- see below) mixed, m = 2 21/30 sp 1.00 3/30 sp 0.50 mixed, m = 4 26/30 sp 1.00 7/30 sp 0.25 "?" primary, m = 2 6/30 sp 0.50 1/30 sp 0.50 "?" primary, m = 4 18/30 sp 0.50 3/30 sp 0.50 Unchanged, and now covered by the oracle: Figure 1 scores exactly 7 (left) and 5 (right) at alpha = 1, and for an all-present block the score is Fitch(primaries) + (alpha/m) * sum_j FitchLen_j exactly -- under ANY most-parsimonious reconstruction of secondary j the number of branches on which j changes is FitchLen_j, so that case was already rooting-invariant whichever labelling the uppass picked. The residual dependence is therefore confined to blocks with mixed present/absent primaries, and its remaining mechanism is the DELTRAN uppass direction; it is NOT closed by this commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The HSJ score is defined as a minimum over internal-node labelings of a sum of SYMMETRIC dissimilarities over the branches of an UNROOTED tree (Hopkins & St John 2021, p.3, p.6), so it may not depend on where the tree is rooted. The a(n)/p(n) DP already satisfied that -- its branch costs are symmetric and it minimises over the root's own state, which is why alpha = 0 was invariant and why an all-present block was invariant at every alpha. The secondary labelling did not: fitch_label_char() resolved ambiguous nodes with a DELTRAN-style uppass whose direction, and with tie-break support counts whose subtrees, are both properties of the INPUT rooting. Two rootings of one topology therefore disagreed about d(u, v), and so about the score. The labelling passes now run over a traversal rooted at tip 0, built once per block and shared across its secondaries. Tip indices come from the dataset, not from the rooting, so this is canonical; neighbour lists are sorted by node index so the traversal cannot inherit the incoming orientation. The kernel's own root becomes an ordinary degree-2 node, which Fitch passes through transparently, so where it sat no longer matters. Tip 0 is a canonical root that carries an observation, so the downpass honours its own state and it resolves within that rather than within a downpass set. This is NOT the reporting-boundary canonicalisation PR #278 applied to XFORM. That left search optimising a root-dependent objective and reconciled the number afterwards. This sits inside the kernel, so the objective the search optimises is itself a function of the unrooted topology, and MaximizeParsimony agrees with TreeLength by construction rather than by re-scoring. It buys invariance, not exactness: the exact minimum over labelings is the inapplicable-Fitch problem and is not available in linear time (a composite Sankoff over {absent} + all secondary combinations would be exact and is likewise rooting-invariant, but it is exponential in the number of secondaries and is what the XFORM path already does). Measured over every edge-rooting of 30 random 9-tip topologies at alpha = 1, root-dependent topologies / worst spread: regime 93c81a9 + inapplicable fix + this all-present 0/30 0.00 0/30 0.00 0/30 0.00 mixed, m = 2 21/30 1.00 3/30 0.50 0/30 0.00 mixed, m = 4 26/30 1.00 7/30 0.25 0/30 0.00 "?" pri, m = 2 6/30 0.50 1/30 0.50 0/30 0.00 "?" pri, m = 4 18/30 0.50 3/30 0.50 0/30 0.00 T-374's headline symptom goes with it. Twelve HSJ searches on 16-tip mixed matrices: at 93c81a9 and after the inapplicable fix alone, 3/12 returned trees whose TreeLength() disagreed with the reported score (worst gap 0.75, one pool spanning 31.25/31.5/31.75/32); now 0/12, and every pool is internally consistent. Unchanged: Figure 1 scores 7 (left) and 5 (right) at alpha = 1, and an all-present block still scores Fitch(primaries) + (alpha/m) * sum_j FitchLen_j exactly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d symptom Six tests, all phrased against what the paper requires rather than against how the kernel computes it. Verified against a build of the pre-fix tip 93c81a9 in a throwaway worktree (testthat::set_max_fails(Inf) -- the default cap of 10 silently hid the last test on the first attempt): rooting invariance, fixed mixed block 2 failures (alpha 0.5 and 1; alpha 0 passes, as the control showing the dependence was entirely in the alpha*d/m term) rooting invariance, 8 random matrices 6 failures "-" vs "?" at an inapplicable secondary 5 failures MaximizeParsimony pool vs reported 5 failures and all four pass after the fix, over repeated runs. Two further tests are regression FLOORS, not witnesses: they pass before and after, and are here to catch a fix that buys invariance by breaking the parts that were already right. - Figure 1 scores 7 (left) and 5 (right) at alpha = 1. - An all-present block equals Fitch(primaries) + (alpha/m)*sum_j FitchLen_j. Notes on two tests that had to be rebuilt before they tested anything: The "-" vs "?" test first used the p.5 "at most 1 per branch" bound. Summed over a tree that bound is far too loose to notice the defect and it passed against the pre-fix build, so it was replaced by the sharper invariant that "-" and "?" at a tip whose primary codes absent are two spellings of "this character does not apply here" and must score alike. Its matrix was searched for specifically because it separates the two codings at a FIXED rooting, so it fails independently of the rooting sweeps. The MaximizeParsimony test was first bounded by maxSeconds and was flaky in both directions: under a wall-clock bound the returned pool depends on machine speed, so whether a discordant tree is in it varies between runs, and it passed pre-fix as often as not. It is now bounded by maxReplicates, on a 14-tip construction where 14 of the first 40 seeds are discordant pre-fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r oracle
Four comments asserted, correctly at the time, that HSJ lengths are
rooting-relative. They are now false for HSJ and still true for XFORM, so each
is narrowed rather than deleted: ts_tbr.cpp's reroot_at_tip() contract and its
reroot-cycle note, ts_rcpp.cpp's ts_collapse_pool re-root, and
MaximizeParsimony.R's collapse note. The XFORM rescore at :1709 is already
gated on useXform, so HSJ correctly does not go through it and no code changes.
ts_tbr.cpp:802-818 was already root-dependence-aware and is untouched.
hsj-paper-oracle.R gains the two checks the row needed and a correction it
cost a round of work to learn:
[1] Figure 1's ABSOLUTE scores, 7 and 5 at alpha = 1, not just the
primaries-only control. Re-derived as 6 + alpha and 3 + 2 * alpha.
[3] Rooting invariance now sweeps all 2n-3 edge-rootings, not n tip-rootings.
[3b] The DISCRIMINATING case: mixed present/absent blocks. Section [3]'s
Fig.1 matrix is ALL-PRESENT, which check [5] shows was already invariant
before any of this, so on its own it could only ever have been a
necessary condition.
[5] The all-present closed form as an explicit regression FLOOR.
Measured: 12 pass / 1 fail at the pre-fix tip 93c81a9 (only [3b] fails, 9/12
mixed blocks dependent, worst spread 0.667), 13 pass / 0 fail after. Every
other check is identical across the two builds, which is the evidence that the
fix buys invariance without disturbing what was already right.
The header's old corollary -- "the paper prescribes a two-state DP and the code
does not implement it" -- is corrected. score_hierarchy_block()'s a(n)/p(n)
recurrences ARE Algorithm 1 lines 6-7 term for term; the DP was already there
and already invariant. Copying Algorithm 1 literally would not have helped
either: its line 2 labels nodes by the Fitch DOWNPASS, which is root-dependent,
and it takes "Tree, T, with root r" as input, so Theorem 2's claim that it
returns the minimal score cannot hold as stated.
NEWS records the user-visible change: HSJ scores on data mixing present and
absent primaries may differ from previous versions and will generally
decrease, the old value having included spurious inapplicable mismatches.
Unlike the XFORM entry above it, this one changes what the search optimises.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ms609
added a commit
that referenced
this pull request
Aug 2, 2026
Records the fix, and corrects two things this row was itself propagating: - "implement the paper's two-state DP" was already satisfied -- score_hierarchy_block()'s a/p recurrences ARE Algorithm 1 lines 6-7 term for term, and were already rooting-invariant. The defect was that d(u,v) was read off the secondary labelling. Copying Algorithm 1 literally would not have helped either: its line 2 labels by the Fitch downpass and it takes a rooted tree as input, so Theorem 2's minimality claim cannot hold as stated. - The 165-197/300 and "24.5/25" figures were measured at 1a94403 and are superseded; the oracle's "2 pass / 3 fail" is stale since PR #280. Also records the fact that localised the bug -- for an all-present block the score is Fitch(primaries) + (alpha/m) * sum_j FitchLen_j exactly under any MPR, so only mixed present/absent blocks could ever be affected -- the two mechanisms, the measurements either side, why the canonical rooting was chosen over an exact composite Sankoff, and why it is not PR #278's reporting-boundary canonicalisation. The XFORM MPT-set residue stays open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review catch on the previous two commits. `pri_free` used the lax test "the primary MAY code absent", which is true for "?" as well as "0" and "-". That wildcarded the secondaries of a tip whose primary is ambiguous even when those secondaries were OBSERVED -- and observing a secondary is itself evidence the structure is present, so the alpha term was discarding real data. Worse, it was self-erasing: fitch_label_char()'s `domain` unions only the non-free tips, so a block whose primaries are all "?" would have had an empty domain and a silently zero alpha term. ValidateHierarchy whitelists "?" primaries, so that data is reachable. It also contradicted the repo's own precedent: recode_hierarchy.R's `tipStates == -2L` / `tipSecKnown` path exists under T-379 for exactly this reason -- an ambiguous primary must not free the secondaries that WERE observed. The test is now strict: free the secondary iff the primary cannot be present, `(set & ~absent_bits) == 0`. Invariance is unaffected -- the inapplicable leak this fix chain closes enters only from tips that are DEFINITELY absent -- and throwing information away was never what bought it. Re-measured over every edge-rooting of 30 random 9-tip topologies, alpha = 1, now crossed over multistate secondaries as well: 0/30 dependent, spread 0.0000 in all twelve cells (present / mixed / "?"-primary, m = 2 and 4, binary and 3-state secondaries). The "?"-primary and multistate axes were missing from both the test suite and the oracle and are now in both. New coverage, verified against a 93c81a9 build: the random-matrix rooting test goes to 13 failures pre-fix (it now crosses 4 combinations), and the oracle's mixed-block check to 4 failures, worst spread 1.0000 on 3-state secondaries -- which the binary-only sweep had not shown. Post-fix: tests 0, oracle 17/0. The new "observed secondary at a '?' primary still counts" test passes BOTH sides and is labelled in the file as a floor rather than a witness: before T-374 no secondary was freed at all, so only the fix could regress it. It pins the narrowness of the rule, because discarding information is perfectly rooting-invariant and would have passed every other test here. NEWS softened to what was actually measured. "Will generally decrease" was an inference, not a measurement; differencing the two builds at each tree's own rooting over 180 tree-matrix pairs gives 152 unchanged, 27 decreased (by up to 1) and one INCREASED by 0.25 -- a rise is possible because the score is now taken at a fixed canonical rooting rather than at whichever rooting the tree arrived in, and that was occasionally the flattering one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
No behaviour change. fitch_label_char()'s header had ended up split across the CanonOrder struct introduced two commits ago, so the T-375 token-index note read as documentation of the traversal; each block now sits above what it documents. The pri_free note was also left describing the lax "may be absent" rule that the previous commit replaced with the strict "cannot be present" one, and now points at score_hierarchy_block() where the reasoning lives. CanonOrder::parent was assigned during the DFS and never read -- an n_node allocation per block per scored tree for nothing. Verified unchanged: 422 HSJ/hierarchy/xform tests pass, oracle 17/0.
ms609
added a commit
that referenced
this pull request
Aug 2, 2026
Status hygiene after the merge, plus one substantive addition: the T-374 row now ends by naming the ONLY thing still live on it, so it is not mistaken for a fully open P1. That is the XFORM MPT-set residue -- pool membership selected at R/MaximizeParsimony.R:1618 on search-time scores taken at differing rootings, so an XFORM pool can span several lengths at a common rooting. Closing it means canonicalising pool SELECTION, which moves n_topologies semantics and interacts with collapse; unanalysed by the T-374b decision document and deliberately out of both T-385's and PR #281's scope. HSJ is now unaffected by any of that: its score no longer depends on the rooting, so it needs no reporting-side reconciliation. Co-Authored-By: Claude Opus 5 <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
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.
Closes the HSJ half of red-team finding T-374 (P1). Stacked conceptually on
PR #280 (T-375/T-376), which had to land first for the primary term to be a
function of the data at all.
What was actually wrong
The brief's premise needs correcting, and it changed the shape of the work.
score_hierarchy_block()'sa(n)/p(n)recurrences already are thepaper's Algorithm 1 (p.15), lines 6-7, term for term. The two-state DP was
implemented, and it was already rooting-invariant: its branch costs are
symmetric and it minimises over the root's own state. No DP rework was needed.
The defect was that
d(u, v)— the secondary dissimilarity — was read offfitch_label_char()'s directional resolution. Two distinct mechanisms:"-"was admitted as an ordinary state of a secondarycharacter. Where a controlling primary codes a structure absent its
secondaries do not exist, so
"-"there is not a state the character takes.Admitting it let the uppass propagate
"-"inwards and label a node in themiddle of the present region "inapplicable", where it is disjoint from
every present neighbour in every secondary at once — charging that branch
d = m, the full alpha. That over-charge is wrong under any rooting; thepaper's
dcounts nonmatching secondaries among characters that apply(p.5).
direction, and tie-break support counts whose subtrees, were properties of
the input rooting.
Copying Algorithm 1 literally would not have fixed this either: its line 2 sets
L(n)to the first-pass Fitch labelling — a downpass, hence root-dependent —and it takes "Tree, T, with root r" as input. Theorem 2 claims it returns the
minimal score, which would make it rooting-invariant; the measurements below are
evidence against that claim as stated.
Key measurement that localised it
Under any most-parsimonious reconstruction of secondary j, the number of
branches on which j changes is
FitchLen_j. So for an all-present blockthe alpha term is
(alpha/m) * sum_j FitchLen_jexactly, whicheverlabelling the uppass picks — that regime was always invariant. Confirmed 50/50
on random data, and it reproduces Fig. 1 exactly (
6+alphaand3+2alpha, i.e.7 and 5 at alpha = 1). The bug could therefore only ever surface on blocks
with mixed present/absent primaries, which is what collapsed the design space.
Root-dependent topologies / worst spread, over every edge-rooting of 30
random 9-tip topologies at alpha = 1:
93c81a9a?primary, m = 2?primary, m = 4Re-run crossed with 3-state secondaries after review: 0/30 and spread 0.0000 in
all twelve cells.
Headline symptom, closed. Twelve HSJ searches on 16-tip mixed matrices: 3/12
returned trees whose
TreeLength()disagreed with the reported score (worst gap0.75, one pool spanning 31.25/31.5/31.75/32) both at
93c81a9aand after fix1 alone; now 0/12, every pool internally consistent.
Why canonical rooting, and why that is not PR #278 again
The exact minimum over labellings is the inapplicable-Fitch problem and is not
available in linear time. (The per-branch min-over-MPR relaxation is refuted: a
0,1,0,1caterpillar gives sum-of-min 0 against min-of-sum 2.) A compositeSankoff over
{absent}plus all secondary combinations, with the symmetric HSJcost matrix, would be exact and is likewise rooting-invariant — the matrix
satisfies the triangle inequality, so even edge-subdivision is safe — but it is
exponential in the number of secondaries and is what the XFORM path already
does; it would delete HSJ's reason to exist alongside XFORM. So the labelling
passes are rooted canonically at tip 0 inside the kernel.
This is deliberately not PR #278's reporting-boundary canonicalisation, which
the brief rightly pre-empted. That left search optimising a root-dependent
objective and reconciled the number afterwards. This makes the objective the
search optimises a function of the unrooted topology, so
MaximizeParsimonyagrees with
TreeLengthby construction rather than by re-scoring. It buysinvariance, not exactness. The option was put to the maintainer with the
measured residual attached and chosen explicitly.
Scope of the freeing rule (review catch)
A secondary is freed only where its primary cannot be present, not merely
where it may be absent. The lax test would wildcard the secondaries of a
"?"primary even when they were observed — discarding real evidence that the
structure is present — and would be self-erasing, since a block of all-
"?"primaries would then have an empty applicable domain and a silently zero alpha
term.
ValidateHierarchywhitelists"?"primaries, so that data is reachable.This matches
recode_hierarchy.R'stipStates == -2L/tipSecKnownpath,added under T-379 for the same reason.
Tests
In
test-ts-hsj.R, all phrased against what the paper requires. Verifiedagainst a build of
93c81a9ain a throwaway worktree — notetestthat::set_max_fails(Inf), as the default cap of 10 silently hid the lasttest on the first attempt:
"0"/"?"primaries"-"vs"?"at an inapplicable secondaryMaximizeParsimonypool vs reported scoreThree are regression floors (pass before and after, to catch a fix that buys
invariance by breaking what was right): Fig. 1 = 7/5; the all-present closed
form; and "an observed secondary at a
?primary still counts", which pins thenarrowness of the freeing rule — discarding information is perfectly
rooting-invariant, so no other test here would have caught the lax version.
Two tests had to be rebuilt before they tested anything. The
"-"vs"?"testfirst used the p.5 "at most 1 per branch" bound, which summed over a tree is far
too loose and passed against the pre-fix build. The
MaximizeParsimonytestwas first bounded by
maxSecondsand was flaky in both directions; it is nowbounded by
maxReplicates, on a construction where 14 of the first 40 seeds arediscordant pre-fix.
hsj-paper-oracle.Rgoes 13 pass / 4 fail at93c81a9ato 17 / 0, with everyother check identical across the two builds. Its rooting check now sweeps all
2n-3edges and adds the discriminating mixed-block cases; its old headercorollary about the missing two-state DP is corrected.
Other
(
ts_tbr.cppx2,ts_rcpp.cpp,R/MaximizeParsimony.R).ts_tbr.cpp:802-818was already correct and is untouched.TreeLength,both builds); the canonical order is built once per block, not per character.
pre-existing
test_dir-outside-R CMD checkartefacts.windows-latest (release)andEasyTrees shinytest2fail on thecpp-searchbase branch too, in the "Set up R dependencies" step —pre-existing and unrelated to this PR.
Measured by differencing the two builds at each tree's own rooting over 180
tree-matrix pairs: 152 unchanged, 27 decreased (by up to 1), 1 increased by
0.25 — a rise is possible because the score is now taken at a fixed
canonical rooting rather than at whichever rooting the tree arrived in.
🤖 Generated with Claude Code