fix(hsj): translate token indices to state indices in HSJ scoring (T-375, T-376) - #280
Open
ms609 wants to merge 11 commits into
Open
fix(hsj): translate token indices to state indices in HSJ scoring (T-375, T-376)#280ms609 wants to merge 11 commits into
ms609 wants to merge 11 commits into
Conversation
…(T-375, T-376) HSJ's primary_present test and fitch_label_char() both compared tip_labels' token (allLevels/contrast-row) indices directly against state (levels) indices -- absent_state, inapp_state, and bit positions in state_sets are all in state space, but tip_labels is in token space. The two spaces coincide often enough that earlier fixes and their test batteries (which permuted levels, not allLevels) stayed green, but on real reductively-coded data (e.g. Vinther2008.nex) absent/inapplicable primaries could read as present while "?" read as absent, and the score was provably not a function of the data under contrast-row permutation alone. DataSet now stores token_states (the token -> state-set bitmask already computed transiently in build_dataset()) and n_levels, so the HSJ kernel can translate a token label into its state set before testing set membership against absent_state/inapp_state, and before building Fitch state_sets for secondary characters (fixing "?" scoring as a concrete, conflicting state instead of a wildcard). Also corrects the stale "token index" docstrings on .HSJAbsentState() and its caller, which described a levels index.
…evels (T-375, T-376) make_hsj_dat()'s construction keeps allLevels in the same relative order as levels, so none of the existing absolute-value tests could have caught the token/state index confusion -- confirmed by running this file's original tests unmodified against a pre-fix build (all still pass, unchanged). Adds: (1) contrast-row permutation invariance, lifted from dev/red-team/heavy-tests/hsj-token-permutation.R, for zero and one secondaries; (2) a MatrixToPhyDat()-emergent token/state misalignment (levels = "- 0 1", allLevels = "1 0 -", the same shape as the shipped Vinther2008.nex dataset) with a hand-derived absolute score, plus the paper's alpha=0-equals-Fitch identity on the same data. Verified both new tests fail against a throwaway pre-fix build of cpp-search and pass against the fix.
Unrelated to the HSJ fix: R/MaximizeParsimony.R's @examples block was updated by an earlier commit (18397db, "regenerate a MaximizeParsimony example left stale by the effort rename") but man/MaximizeParsimony.Rd wasn't regenerated to match. Caught by running devtools::check_man() before dispatch, as this branch's fix required no doc changes of its own.
…374 fix Verified while validating the T-375/T-376 fix: check [3] now runs live (the alpha term is no longer inert) and passes on this small, Fig.1-derived matrix -- but a fresh sweep over larger/messier matrices shows T-374's rooting-dependence still reproduces readily post-fix, exactly as expected since T-374 needs the paper's two-state DP, which this fix does not touch. Add a comment so a future session doesn't read an all-green run here as T-374 being closed.
ms609
added a commit
that referenced
this pull request
Aug 1, 2026
Both fixed together on feature/hsj-token-index-fix. T-374 (HSJ rooting) remains open and unaffected; noted that hsj-paper-oracle.R now reads 8 pass/0 fail (not 2/3) so a future T-374 session doesn't mistake that for a T-374 fix.
…olution invariant)
The permutation and misalignment tests added earlier both use hsj_alpha=0,
which never calls count_mismatches() -- they exercise T-376's
primary_present term exclusively and can't see whether fitch_label_char()
resolves an ambiguous secondary correctly. This isolates T-375 with all
primaries "1" (so the primary DP is loss-free and the whole score reduces
to the secondary's Fitch step count) and checks T-375's own stated
acceptance criterion: score("?") <= min over concrete resolutions.
Verified to fail against a throwaway pre-fix build (scored 1, violating
the invariant against min(0,1,1)=0) and pass post-fix.
Sharpened by the T-375/T-376 fix: a bad value used to be a harmless wrong scalar comparison, but now indexes DataSet::token_states directly (out-of-bounds read) or shifts by absent_state (undefined for >= 32). Only the internal ts_hsj_score() test bridge is reachable with hand-crafted values -- public callers always derive tip_labels/absent_state from a validated phyDat -- so this mirrors the existing validate_tip_data_values guard rather than changing any public behaviour.
…in NEWS hsj-paper-oracle.R's earlier caveat was a source comment on the branch that never executes when check [3] passes -- a future session reading stdout would miss it. Moved into a cat() so it prints on every passing run. NEWS.md: HSJ scores under inapplicable = "hsj" can change from previous versions now that the controlling primary's presence/absence and an ambiguous secondary are read correctly -- log it next to the existing x-transformation rooting entry, same "To integrate into 2.0.0" convention.
…ath too /code-review (xhigh) flagged this independently across 6 of 10 review angles: validate_hsj_tip_labels() only guarded the ts_hsj_score() test bridge, leaving unpack_hsj() -- which populates the same fields for the real ts_driven_search()/ts_collapse_pool() search path -- completely unvalidated. This is concretely reachable, not just hypothetical: R/ts-driven-compat.R's legacy flat-argument wrapper exposes hsjTipLabels/ hsjAbsentState as independent, hand-settable arguments feeding straight into unpack_hsj(). Since this PR removed fitch_label_char()'s old defensive clamp in favour of direct token_states[label] indexing, a bad value here is now an out-of-bounds heap read instead of a harmless wrong comparison. Also tightens the absent_state bound from a hardcoded [0,31] (shift safety only) to [0, n_levels) -- the dataset's actual state count -- since a value in between passed silently but reproduced the exact T-375/T-376 symptom class (the primary is never classified absent). Adds a default initializer to HierarchyBlock::absent_state for defensive symmetry with DataSet::inapp_state's existing default. Verified: a hand-crafted out-of-range hsjTipLabels/hsjAbsentState now raises a clean Rcpp::stop() from all three entry points (ts_hsj_score, and now ts_driven_search via the compat wrapper), where it previously either mis-scored silently or would have read out of bounds.
/code-review flagged this independently via two angles: sizing fitch_label_char()'s tb_cnt/tb_mintip arrays by the dataset-global n_levels (instead of the old per-character state count) meant a dataset with a high global state count but binary hierarchy secondaries paid up to ~16x more allocation/scan cost per secondary character per HSJ full-rescore call than before this PR. The downpass only ever intersects/unions existing tip bits, so no bit outside the union of tip state-sets can appear at any node -- accumulate that union while initializing state_sets (one extra OR per tip, already being read) and size K from its highest set bit instead of the global n_levels. Restores the old tightness with no extra tree traversal. Also replaces the tie-break credit loop's manual "scan for the single set bit" with ctz64() (already declared in ts_data.h, already used elsewhere in this codebase for the identical operation), removing a 7-line loop-with-break in favour of one call. No test changes needed: this is a pure sizing/lookup optimisation, not a behaviour change -- full test-ts-hsj.R (85 assertions) and the hsj-token-permutation.R/hsj-paper-oracle.R heavy tests still pass unchanged.
…ology, vignette /code-review (xhigh) findings addressed: - NEWS.md's new HSJ entry called T-374's rooting-dependence "intentional" (contradicts this repo's own settled finding that it's a tracked bug per the Hopkins & St John paper) and claimed scores are "generally increasing" (contradicted by this PR's own T-375 test, which shows a decrease case). Corrected both. - .BuildTipLabels()'s own docstring (R/CharacterHierarchy.R) still called its output "state labels" -- the exact token/state conflation this PR fixed everywhere else except at the one place that actually produces the token-space data. - A pre-existing test helper comment (test-ts-hsj.R) called absent_state a "token index"; it is a state index, per this PR's own terminology fix elsewhere. - A new test's comment + expect_equal info= string cited a hardcoded ts_hsj.cpp line number that had already drifted from the code it described (and would surface the wrong line in CI failure output). - ts_sector.cpp's two T-303 guard-rationale comments enumerated the DataSet fields build_reduced_dataset() doesn't copy, but didn't mention the two fields (token_states, n_levels) this PR added. - vignettes/search-algorithm.Rmd wasn't updated despite AGENTS.md's mandatory-checks rule for any scoring-behaviour change (this PR's own NEWS.md entry documents exactly that); added a short note that HSJ scoring is correct regardless of a dataset's internal token ordering. Also refactors the new contrast-row-permutation test's two near-identical permutation-generation blocks into one shared .AllTokenOrderingScores() helper, called twice -- flagged as duplicated logic that would otherwise need fixing in two places.
GHA's R-CMD-check caught what my local spell-check missed: "HSJ's" isn't in inst/WORDLIST (only bare "HSJ" is -- spelling::spell_check_test() doesn't stem across the apostrophe, same reason "TNT's" needed its own separate WORDLIST entry elsewhere in this file). Reworded rather than added a wordlist entry, per AGENTS.md's stated preference. My own verification gap: I'd been running spelling::spell_check_package(vignettes = FALSE) all session, which apparently doesn't scan NEWS.md the same way tests/spelling.R's actual spell_check_test(vignettes = TRUE) does. Verified against that exact invocation this time; both spell_check_package() (default args) and spell_check_test(vignettes = TRUE) now pass clean.
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
1 participant
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.
Summary
Fixes red-team findings T-375 and T-376 in the HSJ (Hopkins & St John 2021) scorer — one defect family, fixed together as required.
.HSJAbsentState()/ds.inapp_stateare STATE (levels) indices, buttip_labels(built by.BuildTipLabels()) holds TOKEN (allLevels/contrast-row) indices.primary_presentandfitch_label_char()'s state-set construction both compared/used these directly, conflating the two index spaces. This is invisible wheneverlevelsandallLevelshappen to share the same order (common, but not guaranteed) — which is exactly why earlier fixes and their test batteries stayed green.fitch_label_char()built Fitch state-sets as1u << label(label = token index), so"?"scored as one arbitrary concrete state instead of the wildcard it denotes.primary_present = (label != absent_state) && (label != inapp_state)compared a token index against two state indices, so the HSJ score was not a function of the data (varied across contrast-row permutations of an otherwise-identical dataset), and on the package's own shippedVinther2008.nex(levels = "- 0 1 2 3",allLevels = "1 ? 0 - 2 3") absent/inapplicable primaries read as present while"?"read as absent.Fix
build_dataset()already computes a token→state-set bitmask transiently (contrast_r→token_states); it's now stored onDataSet(token_states+n_levels) instead of discarded. The HSJ kernel translates a tip's token label through this bitmask before comparing it toabsent_state/inapp_state, replacing:fitch_label_char(), and==inprimary_present, now genuine set membership (a[t]=0iff the token's state set meets the absent-coding states,p[t]=0iff it meets the present-coding states — so"?"correctly givesa=p=0, not a forced classification)..HSJAbsentState()is unchanged (it already returned the correct thing — a state index — despite a stale docstring calling it a "token index", now fixed). No R-visible or Rcpp-exported signature changed.Out of scope, deliberately: T-374 (HSJ rooting-dependence) is untouched. HSJ's rooting-invariance requirement needs the paper's two-state DP, which this PR does not implement — canonicalising rooting here would trade a wrong objective for a stably-wrong one. Confirmed T-374 still reproduces post-fix on other data; added a caveat to
hsj-paper-oracle.Rso a future session doesn't mistake its now-passing rooting check (on that script's own small matrix) for a T-374 fix.Test plan
dev/red-team/heavy-tests/hsj-token-permutation.R: exit 1 (bug reproduces) pre-fix → exit 0 post-fix, both sweeps.dev/red-team/heavy-tests/hsj-paper-oracle.R: 2 pass/3 fail pre-fix → 8 pass/0 fail post-fix.test-ts-hsj.R(contrast-row permutation invariance; aMatrixToPhyDat-emergent misalignment with a hand-derived absolute score) — verified to fail against a throwaway pre-fix build ofcpp-searchand pass post-fix.test-ts-hsj.Rsuite (21 tests) plustest-tree_length.R,test-ts-t330-collapse-hsj-xform.R,test-ts-resample-hierarchy.R,test-ts-prune-reinsert.R,test-ts-t306-accept-guard.R,test-ts-xform.Rpass unchanged.token_states/n_levelsbeing empty on abuild_reduced_dataset()result is safe: every HSJ/XFORM scoring path already bails before reaching a reduced dataset (same guards as T-303).spelling::spell_check_package()clean;devtools::check_man()clean (regenerated one stale, unrelated.Rdexample left behind by an earlier commit already oncpp-search).R-CMD-checkgreen on all platforms (Linux/Windows/macOS, release/devel/ARM, incl. coverage jobs): https://github.com/ms609/TreeSearch/actions/runs/30698271349🤖 Generated with Claude Code