Skip to content

fix(hsj): an inapplicable secondary contributes nothing to the dissimilarity; nested hierarchies validate (T-395, T-396) - #283

Open
ms609 wants to merge 2 commits into
cpp-searchfrom
feature/hsj-paper-conformance
Open

fix(hsj): an inapplicable secondary contributes nothing to the dissimilarity; nested hierarchies validate (T-395, T-396)#283
ms609 wants to merge 2 commits into
cpp-searchfrom
feature/hsj-paper-conformance

Conversation

@ms609

@ms609 ms609 commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Closes the two red-team findings whose fix turned on a question about the method rather than about the code. Both are settled against Hopkins & St John (2021), which is decisive on each.

Note on the diff: this branch also carries e56161aa, the area-10 red-team round record, because cpp-search has not been pushed yet. Push cpp-search and the diff here collapses to the single code commit b2f551be.

T-396 — an inapplicable secondary was scored as a state

Where a secondary was coded "-" but its controlling primary did not certainly code the structure absent — primary "?", or a matrix that codes the structure present and the secondary inapplicable anyway — fitch_label_char() kept "-" as an ordinary concrete state. Being disjoint from every other state, the uppass could propagate it inwards and label an internal node in the middle of the present region with it, mismatching every secondary of the block at once, so score_hierarchy_block() charged that branch d = m — the full alpha — for a node that by construction has no inapplicable secondaries.

Why the paper decides this, and against the shipped test. d counts "nonmatching secondary characters" among those that apply; where secondaries are inapplicable to a taxon "they have no influence on the estimated dissimilarity" (p.5); and treating inapplicable cells as a separate state "increases the dissimilarity of all pairwise comparisons … [resulting] in overweighting the [controlling] primary character and favors clades that separate taxa with secondary characters from those without" (p.5). That is the FitchS behaviour HSJ exists to replace, so admitting "-" was not a stricter reading of HSJ — it was the thing HSJ is defined against.

inapp_bit is now stripped from every secondary's state set at every tip, and a cell left with no applicable state becomes non-constraining.

This changes a shipped test, deliberately. test-ts-hsj.R:857 asserted c(0, 1, 1, 0) over c("0", "1", "-", "?"), with a comment rationalising score("-") == 1. That is now c(0, 1, 0, 0). The old value was not a defensible alternative reading: it exceeded every concrete resolution of the cell and the missing-data treatment, so no resolution produced it. Measured over-charge is exactly alpha/m per affected branch (witness: 1 vs 0.5 at alpha = 0.5; 2 vs 1 at alpha = 1).

Scores are unchanged on consistently-coded matrices, and remain rooting-invariant.

T-395 — every nested hierarchy was rejected

.ParseOneBlock() records a sub-controller both as a dependent of its parent and as the controlling character of its own block. That dual role is nesting. .ValidateBlock() counted the second occurrence as a double claim, so the example documented in ?CharacterHierarchy could not be validated, scored or searched.

R CMD check could not catch it: CharacterHierarchy() never validates, so the example constructs fine and is never scored. A child block no longer re-claims its controlling character; genuine double claims — including a nested block's dependents colliding with another block — still error.

HierarchyFromNames() nesting was dead for an independent reason, so fixing only the validator would have left nesting silently flattened: it tested intersect(subIdx, supIdx), necessarily empty since no single name carries both prefixes, and matched tags only as far as their first underscore, collapsing every depth onto the outermost tag. Nesting is now expressed by tag extension — sup_tail_tip extends sup_tail — with longest-match attachment, so sub_tail_tip_gloss belongs to sup_tail_tip rather than sup_tail. A shared prefix without an underscore boundary does not nest (sup_tailfin stays independent of sup_tail). Documented, exampled and tested to three levels.

The x-transformation genuinely does not implement nesting and now reports that directly instead of failing validation first.

Verification

  • New tests fail 6/6 against a baseline build with both fixes reverted, and pass with them. test-ts-hsj.R: 2 failures pre-fix, 150 pass / 0 fail post-fix.
  • Fix gates all green: hsj-paper-oracle 17/17, hsj-token-permutation invariant in both sweeps, hsj-totalwords-zero-noop searches.
  • Nested HSJ scores are rooting-invariant across all 6 rootings.
  • 673 pass / 0 fail across the hsj, hierarchy, xform, sankoff, collapse, recode, tree_length and resample suites.
  • check_man() and spell_check_package(vignettes = TRUE) clean.

Also fixed

Five tests in test-CharacterHierarchy.R called internal functions without a TreeSearch::: prefix and so errored under a temp-library install — including ".HierarchyToBlocks flattens nested hierarchies", the only test of the nested path, which had therefore never executed. It passes.

🤖 Generated with Claude Code

CI status — read before treating the red X as this branch's fault

Run 30827339510: ubuntu-arm64 success, windows failure.

The Windows job failed in Set up R dependencies, before compiling or testing anything:

Could not solve package dependencies:
* deps::.: Can't install dependency MaxMin
* MaxMin: Can't find package called MaxMin.
* any::sessioninfo: dependency conflict

This is pre-existing and unrelated to the diff: MaxMin is declared at DESCRIPTION:61, this branch does not touch DESCRIPTION or any workflow, gha-ccache failed the same way on 2026-08-01, and there is already a branch claude/windows-maxmin-binary working the problem.

ubuntu-arm64 ran a full R CMD check with NOT_CRAN: true — so the test suite genuinely executed rather than being skipped by the file-level skip_on_cran() in test-ts-hsj.R — and reported checking tests ... OK, Status: OK. The only warnings are pre-existing ones in test-MaddisonSlatkin.R and test-ts-profile.R, untouched here.

So the change is validated on the platform that could build it, and Windows coverage is blocked on the separate MaxMin dependency fix rather than on anything in this PR.

ms609 and others added 2 commits August 3, 2026 16:23
…dates

Two red-team findings, both settled against Hopkins & St John (2021) rather
than against the existing tests.

T-396 -- an inapplicable secondary was scored as a state.

Where a secondary was coded "-" but its controlling primary did not CERTAINLY
code the structure absent (primary "?", or a matrix that codes the structure
present and the secondary inapplicable anyway), fitch_label_char() kept "-" as
an ordinary concrete state. Disjoint from every other state, it could be
propagated inwards by the uppass and label an internal node in the middle of
the PRESENT region, mismatching every secondary of the block at once, so
score_hierarchy_block() charged that branch d = m -- the full alpha -- for a
node that by construction has no inapplicable secondaries.

The paper decides this, and against the shipped test: d counts "nonmatching
secondary characters" among those that APPLY, inapplicable secondaries "have no
influence on the estimated dissimilarity" (p.5), and treating them as a
separate state "increases the dissimilarity of all pairwise comparisons ...
overweighting the [controlling] primary character" (p.5) -- which is the FitchS
behaviour HSJ exists to replace, not a stricter reading of it. inapp_bit is now
stripped from every secondary's state set at every tip, and a cell left with no
applicable state is non-constraining.

test-ts-hsj.R:857 pinned the over-charge as expected (c(0, 1, 1, 0), with a
comment rationalising score("-") == 1). That expectation is wrong and is now
c(0, 1, 0, 0); the old value exceeded every concrete resolution of the cell AND
the missing-data treatment, so no reading of the data produced it. Measured
over-charge was exactly alpha/m per affected branch (witness: 1 vs 0.5 at
alpha=0.5, 2 vs 1 at alpha=1).

T-395 -- every nested hierarchy was rejected.

.ParseOneBlock() records a sub-controller both as a dependent of its parent and
as its own block's controlling character; that dual role IS nesting.
.ValidateBlock() counted the second occurrence as a double claim, so the
documented example in ?CharacterHierarchy could not be validated, scored, or
searched -- and R CMD check could not see it, because CharacterHierarchy()
never validates: the example constructs fine and is never scored. A child block
no longer re-claims its controlling character. Genuine double claims, including
a nested block's dependents colliding with another block, still error.

HierarchyFromNames() nesting was dead for an independent reason: it tested
intersect(subIdx, supIdx), which is necessarily empty since no one name carries
both prefixes, and matched tags only as far as their first underscore, which
collapsed every depth onto the outermost tag. Nesting is now expressed by tag
extension (sup_tail_tip extends sup_tail) with longest-match attachment, so
sub_tail_tip_gloss belongs to sup_tail_tip. A shared prefix without an
underscore boundary does not nest. Documented and exampled.

Verification
- New tests fail 6/6 against a baseline build with both fixes reverted, and
  pass with them; test-ts-hsj.R fails 2 pre-fix, 150 pass post-fix.
- Fix gates all green: hsj-paper-oracle 17/17, hsj-token-permutation invariant,
  hsj-totalwords-zero-noop searched.
- Nested HSJ scores are rooting-invariant over all 6 rootings; xform reports
  its own "nested not supported" error instead of failing validation first.
- 673 pass / 0 fail across hsj, hierarchy, xform, sankoff, collapse, recode,
  tree_length and resample suites.
- check_man and spell_check_package(vignettes = TRUE) clean.

Also fixes five tests in test-CharacterHierarchy.R that called internal
functions without a TreeSearch::: prefix and so errored under a temp-library
install, including ".HierarchyToBlocks flattens nested hierarchies" -- the one
test of the nested path, which had never executed. It passes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ms609 pushed a commit that referenced this pull request Aug 5, 2026
Open findings now live as GitHub issues in the agent-issues fork, labelled
red-team + sev:high|med|low + area:N. Status becomes GitHub state, so it can
no longer drift from merge state.

- 24 genuinely-open findings published as #1-#24
- 23 already-fixed rows archived into findings-archive.md, which is now FROZEN
- 2 (former T-395/T-396) not migrated: open upstream PR #283
- migration-map.tsv maps all 49 historical T-nnn to issue / archive / PR
- findings.md deleted; recoverable via `git show 03dc565:dev/red-team/findings.md`

The reconciliation found 23 of 49 rows reading as open when their fixes had
already landed, only four carrying the "awaiting tidy archive" marker. That
drift is the reason for the move, and the README now records it.

T-nnn ids are frozen, not retired: they persist in shipped source comments
(src/ts_collapsed.cpp, src/ts_rcpp.cpp) and throughout log.md. Cross-repo refs
use the fully-qualified agent-issues/TreeSearch#N form; a bare #N means the
fork.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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