Skip to content

fix: derive XFORM secondary state spaces from contrasts, not token strings - #56

Open
ms609 wants to merge 7 commits into
cpp-searchfrom
feature/xform-statespace
Open

fix: derive XFORM secondary state spaces from contrasts, not token strings#56
ms609 wants to merge 7 commits into
cpp-searchfrom
feature/xform-statespace

Conversation

@ms609

@ms609 ms609 commented Aug 5, 2026

Copy link
Copy Markdown

Fixes #11
Fixes #12
Fixes #17

A hierarchy block took its secondary characters' states from the distinct token
strings observed in each column, minus "-" and "?". Three defects followed.

#11 (T-393) — an ambiguity token became a state of its own

"{01}" is neither "-" nor "?", so it survived the filter and was admitted as a
level: one Hamming step from both "0" and "1" rather than matching either. A
polymorphic cell therefore cost a step that no resolution of it requires, and each such
cell multiplied the combination count. Levels now come from the phyDat contrast
matrix, mirroring DataSet::token_states, which the HSJ path already reads.

An unresolved secondary is carried as a bit mask of admissible levels rather than a
single level index, so an ambiguity narrower than the full state space still constrains
the combination. Both C++ consumers (unpack_xform, ts_sankoff_test) were changed
together.

A side effect worth noting: level order was previously locale collation over token
strings, so sort() could order "{01}" before "0". It is now the contrast column
order.

#12 (T-394) — a secondary with no observed state made the whole score Inf

Validation asks that a secondary be coded inapplicable where its primary is absent,
never that any state of it remain observed. Dropping a taxon can therefore leave a
secondary all-gap in a dataset that validated. prod() over a zero-width state space
gave nStates == 1, no tip admitted any state, and TreeLength() returned Inf
silently; MaximizeParsimony() then aborted at the reporting if with
missing value where TRUE/FALSE needed, because diff(range(c(Inf, Inf))) is NaN.

Such a secondary is now carried as one unobserved level: it adds nothing to any tree's
length, but still counts towards its block's gain cost, which the hierarchy fixes rather
than the taxa sampled. The NaN is guarded independently of what produced it, in
.ReportXformScore().

#17 (T-401) — the report block scored its own collapse output

MaximizeParsimony() rescores its returned pool at the canonical rooting. It now scores
the binary pool a contracted tree came from, so it no longer depends on the HSJ/XFORM
collapse no-op staying in place, and warns when it does so — reporting the length of
trees the user is not handed is itself the discrepancy T-385 was filed for.

The comment does not claim the two pools have equal lengths. That is not derivable:
the collapse flags are decided over ds.blocks[], the Fitch term alone, so an
XFORM-blind unblocking would contract branches that are not zero-length under the
Sankoff term.

Intended score changes

X-transformation lengths change on data with polymorphic or partly ambiguous secondary
characters, and do not increase.
A cold review fuzzed ~780 random matrices against a
brute-force "minimum over concrete resolutions" oracle: 0 mismatches. Across 1170
old-vs-new comparisons: 573 decreased, 597 identical, 0 increased. Separately,
lengths that were Inf become finite. Data coded only with unambiguous tokens, ? and
- is otherwise unaffected.

New hard error: a secondary with more than 31 states cannot be recoded (one bit per
level). Documented in NEWS.md, the roxygen and vignettes/inapplicable.Rmd.

Pre-fix failure output

tests/testthat/test-ts-xform-statespace.R against cpp-search @ 99b79d8f
(capped at 10):

2. Failure (:50): Polymorphic secondary costs no more than its best resolution
   `actual`: 4.0   `expected`: 3.0            # length; also n_states 4 vs 3
3. Failure (:70): Polymorphic cells do not inflate the state space
   Expected `RecodeHierarchy(ds, h)` to run silently. Actual noise: warnings.
4. Failure (:71): `actual`: 82.0  `expected`: 17.0
5. Failure (:81): A secondary beyond the mask s width -- expected an error
6. Failure (:118): Polymorphism narrows a multistate secondary
   `actual`: 5.0   `expected`: 4.0
7. Failure (:121): `recoded$tip_sec_known[2, 1]`  `actual`: 0  `expected`: 3
8. Failure (:141): Search and TreeLength agree on a multi-bit secondary mask
   `actual`: 0     `expected`: 3
9. Failure (:180): Unobserved secondary leaves a finite length
   `actual`: Inf   `expected`: 2.0
10. Error (:189): Search over a subset with an unobserved secondary
   Error in if (diff(range(canonicalScores)) > sqrt(...)) :
     missing value where TRUE/FALSE needed

One test in the file passes pre-fix and is not a regression test for these issues:
"Polymorphism narrows a multistate secondary without freeing it"'s length assertion.
It guards against the cheaper fix I did not take — treating any ambiguity as wholly
unknown, which would free the tip to state "2" and under-count. Its n_states and
mask assertions do discriminate, and are listed above.

Both encodings are numerically identical for a binary secondary (1 << (L-1) == L for
L in {1,2}), so the suite would have stayed green had the unpack_xform site been
left unpatched. That hole is now closed by a multi-bit-mask test driving both consumers
end to end.

Validation

ubuntu-arm64 green. Two CI failures are pre-existing on cpp-search, verified by
dispatching the same workflows on trunk:

An earlier revision of this branch did crash ubuntu-arm64 with double free or corruption. That was this PR's own test calling TreeLength() on a polytomous tree
to demonstrate the wrong length — which is precisely the unfixed #16 corruption. The
test no longer performs that call.

Local: 561 targeted tests pass; the new file, test-recode-hierarchy.R and the T-330
file are clean under -D_GLIBCXX_ASSERTIONS. spell_check_package(vignettes = TRUE)
and check_man() clean.

🤖 Generated with Claude Code

ms609 and others added 5 commits August 4, 2026 16:56
…rings

A hierarchy block's secondary levels were the distinct token STRINGS observed
in that column, minus "-" and "?".  An ambiguity token such as "{01}" survived
that filter and became a level of its own: one Hamming step from both "0" and
"1" rather than matching either, so a polymorphic cell cost a step no
resolution of it needs, and each such cell multiplied the combination count
((k + 1)^m present states rather than k^m).  Read the levels from the phyDat
contrast matrix instead, as the HSJ path already does, and carry an
unresolved secondary as a bit mask of admissible levels so an ambiguity
narrower than the full state space still constrains the combination.

A secondary with no observed level -- reachable by dropping taxa from a
dataset that validated, since ValidateHierarchy runs before both subsetting
sites -- gave a zero-width state space, so no tip admitted any state and the
block's length was Inf.  Carry it as one unobserved level: it adds nothing to
any tree, and the gain cost still counts every secondary the primary controls.

MaximizeParsimony() rescores its own returned pool at the canonical rooting.
That call could not survive a polytomous pool (TreeLength scores the topology
it is given, and a contracted tree's length is that of its best resolution) or
a pool with no finite length (diff(range(.)) is NaN, which `if` aborts on).
Score the binary pool the returned trees were contracted from, and reduce the
lengths through a guard that reports rather than branches on non-finite ones.

Fixes #11
Fixes #12
Fixes #17

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addresses review of the x-transformation state-space fix: reporting the
length of the binary pool a contracted tree came from is itself the T-385
discrepancy, so say so; the collapse flags are decided over the Fitch term
alone, so the two pools' lengths are equal only if a future collapse fix is
hierarchy-aware, which the comment no longer assumes.

Also: state the 31-level ceiling and the max(k, 1) state count in the docs and
vignettes, pin the state space and the multi-bit mask that the length
assertions alone did not, and drive both mask consumers end to end.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The T-401 regression test scored a contracted tree directly, to show that
doing so does not reproduce the binary length.  That call is the very defect
the test exists to guard against: until the kernel bounds-checks a non-binary
edge matrix (T-400) it writes past the end of its Fitch word vector, which
tryCatch cannot contain.  It read as a wrong length on x86 and aborted
ubuntu-arm64 with 'double free or corruption'.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ing matrix

The warning fired before the substituted pool was known to be non-empty, so
it could promise a length recomputed from binary trees and then report the
search's own score instead.

`RecodeHierarchy()` built a full taxon-by-character character matrix that,
once the state space came from the contrast matrix, was read only to fetch
each tip's primary token; the integer token matrix already carries it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MaximizeParsimony() derives its reported score by calling TreeLength() on the
pool (T-385), so asserting the two agree is true by construction.  Compare the
search's optimum against a brute force over all six-taxon topologies instead,
and state plainly that what keeps the two mask consumers honest is being
edited together, not this test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ms609

ms609 commented Aug 5, 2026

Copy link
Copy Markdown
Author

Final validation, on 21de433f8 (run 30971389983):

  • ubuntu-arm64: success.
  • windows: fails in Set up R dependencies (error in pak subprocess), identical to cpp-search — infrastructure, no package code reached.

Two commits since the PR body was written, both from review:

  • fe79c502 — the polytomy warning fired before the substituted pool was known to be non-empty, so it could promise a length recomputed from binary trees and then report the search's own score. RecodeHierarchy() also still built a full taxon-by-character character matrix that, once the state space came from the contrast matrix, was read only for each tip's primary token; the integer token matrix already carries it.

  • 21de433f — the test named "Search and TreeLength agree on a multi-bit secondary mask" held by construction: since T-385 MaximizeParsimony() derives its score by calling TreeLength() on the pool, so the assertion was the same computation twice and reverting the unpack_xform half would have left it green.

    No stronger assertion is available on that fixture: brute-forcing all 105 six-taxon topologies gives optimum 3 under {01} and under each of 0, 1, 2, ?, so neither an optimum comparison nor a lower bound on replicate_scores separates the two readings. The test now asserts against that independently brute-forced optimum, is renamed to what it does, and says plainly that it does not establish agreement between the two mask consumers. What keeps those consistent is that they are edited together, recorded in .AGENTS/memory/feature-inapplicable.md.

So one claim in the PR body above is overstated and worth reading with this correction: the multi-bit-mask test closes the coverage hole (a multi-bit mask is now produced and driven through the search) but does not prove the two consumers agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment