Fix four Consistency.R bugs: token remapping, cache collision, matrix drop, NaN docs

[ms609]

Summary

Four fixes in R/Consistency.R, filed by /red-team area 14:

• .SortTokens() rewrote a partial-ambiguity token as full ambiguity (sev:high). The ambiguous-token remap indexed its right-hand side by every ambiguous token in the dataset's contrast, not just the ones present in the character being processed. When a character used only one of several ambiguous tokens in the contrast (e.g. only (01), never ?), the mismatched-length assignment silently (or with a cryptic warning) rewrote it to the wrong value.
• ExpectedLength()'s cache key omitted the tree. Two different trees scored against the same dataset with the same nRelabel collided on the same cache entry, so the second tree's Consistency() call could silently return the first tree's rhi.
• Consistency() dropped to a bare vector for a 1-pattern dataset, missing drop = FALSE.
• Undocumented NaN returns for constant/autapomorphic/zero-null-homoplasy characters — documented in @return, values unchanged.

Fixes #88
Fixes #87
Fixes #94
Fixes #112

Pre-fix failures

Each new test was confirmed to fail against the original code before its fix (verified by stashing the R/Consistency.R fix and re-running):

• #88 test: returned 15 (wrong) with a "not a multiple of replacement length" warning instead of 6.
• #87 test: pectinate-tree query returned the balanced tree's cached 4 instead of the correct 5.
• #94 test: Consistency() returned a bare numeric vector (dim = NULL) instead of a 1x4 matrix.
• #112's test is coverage for newly-documented (but code-unchanged) behaviour, not a pre-fix failure in the strict sense — the NaN values themselves were already correct; only the docs were missing. It does incidentally fail pre-A14-22: Consistency() returns a bare vector, not the documented matrix, for a 1-pattern dataset #94-fix too, since it indexes the same single-character Consistency() result with [, "ci"].

Two pre-existing tests ("CI & RI calculated correctly", "RHI calculated okay") asserted a bare named vector for a single-character dataset; that was itself an instance of the #94 bug, so they're updated to expect the (correct) 1-row matrix.

Review

Reviewed by three independent external-reviewer passes (AGENTS.md/style compliance, cold bug-scan, git-blame history). Findings acted on:

• Fixed a real inaccuracy in the new @return text: rhi is NaN only when both the observed and median-null length equal the minimum; if only the median equals the minimum, it's Inf, not NaN.
• spelling::spell_check_test() silently no-ops locally without NOT_CRAN=true set (skip_on_cran() treats an unset NOT_CRAN as "on CRAN"); running it properly surfaced autapomorphic as an unlisted word, now added to inst/WORDLIST.
• Wrapped new test-file lines over 80 characters per the lintr house rule.
• Added a discriminating .SortTokens() oracle test and a cache-hit assertion for extra coverage.

One high-severity finding was not fixed here: a pre-existing (not introduced or worsened by this PR — reproduces identically against the original code) crash in .SortTokens()/ExpectedLength() when every state of a character occurs only inside a polymorphism (e.g. "00(12)(12)"), never as its own unambiguous token. Flagged separately for follow-up, out of scope for these four issues.

Test plan

Follow-up: the #87 cache key, reworked

Review of the first cut turned up three further problems with the key, all now fixed.

It was built from the edge matrix verbatim, so the same tree in a different edge order or node rotation missed the cache entirely. Consistency() postorders its tree before calling ExpectedLength(), so a direct ExpectedLength() call on the same tree could not reuse the entry Consistency() had just created.

It pasted the tree component and the state counts into one comma-delimited string, leaving no marker for where one ended and the other began: "100" + "1,2" + c(3, 4) and "100" + "1,2,3" + c(4) both render "100,1,2,3,4", returning a false hit. A tip label containing a comma collided the same way. Nesting a per-character environment inside a per-tree one removes the concatenation altogether, and stops the tree key being copied into every character's entry.

And it identified the wrong thing. The key now identifies the unlabelled rooted shape, which is what the sampled distribution is actually a function of: FastCharacterLength() is positional and never reads tip.label, and the relabellings are uniform, so relabelling composes with a uniform permutation and leaves the distribution unchanged (confirmed against 3000-replicate distributions — balanced vs. relabelled-balanced agree within noise, pectinate is visibly shifted). Identical labelled topologies are a subset of identical shapes, so the labelled key missed every reuse the shape key catches and none of its own.

That reuse is substantial in the workflow that matters. Random trees of 24+ leaves practically never share a shape — but most-parsimonious trees are not random, differing only by local rearrangement. Across the 31 distinct MPTs of inapplicable.phyData[[4]], the 23-taxon dataset this function's own example uses, there are 19 distinct shapes: 39% of the work goes away.

TreeTools::RootedTreeShape() can't serve as the key — it enumerates shapes into an integer64 and stops at 55 leaves, which 16 of the 30 bundled inapplicable.phyData datasets exceed. .ShapeKey() instead builds the Aho–Hopcroft–Ullman canonical code (leaf 01; an internal node wraps its children's codes, sorted into a fixed order, in 0…1) and packs it to bytes: no ceiling, and 12–20× shorter than the labelled key it replaces (n=88: 91 chars against 1609). It is verified against RootedTreeShape() as an equivalence relation over n=4–12 — identical partitions, up to 214 classes — which is the check that guards against a bug silently conflating two shapes and corrupting rhi.

Because sorting the child codes is what makes the encoding canonical, it is inherently invariant to edge order and rotation, so the Preorder(SortTree()) step and its SortTree import are gone again; NAMESPACE is unchanged from trunk. Two details worth knowing: the leaf count has to lead the key, since n=55 and n=56 both pad to 224 bits; and rooting is deliberately part of the shape, as these characters may carry inapplicable tokens, whose lengths are not rooting-invariant.

The cache regression tests assert the key count, not the returned value: the median is stable enough that a recomputation returns the same number, so a value-only test passes pre-fix. Verified failing pre-fix (5 → 6 → 7 keys across three edge orderings, against a constant 1 after).

This branch has been rebased onto the trunk merge, which brought in #142's fix for the .SortTokens() polymorphism crash — a follow-up spun out of this same review. The two changes are complementary and both are exercised by the suite.

Test plan

• Rscript -e "devtools::check_man()" — clean
• spelling::spell_check_package(vignettes = TRUE) — clean (note: spell_check_test() silently no-ops without NOT_CRAN=true, which is how a missing autapomorphic entry initially escaped)
• testthat::test_dir("tests/testthat", filter = "Consistency") — 51/51 pass
• .ShapeKey() cross-validated against TreeTools::RootedTreeShape() as an equivalence relation
• GHA agent-check.yml — PASS on the two-level-cache commit (run 31183125279); re-dispatched for the shape key as run 31186062435

🤖 Generated with Claude Code