docs: state Wagner's equal-weights placement contract; pass min_steps - #55
Merged
Conversation
…te tip_ambiguous plane-0 behaviour AdditionTree() built searchArgs without min_steps, so a finite `concavity` scored ts_wagner_tree's result against min_steps = 0 instead of the dataset's real per-character minimum step counts (MinimumLength()). This corrupted only the internal `result$score`, which AdditionTree() itself discards, and never moved the returned topology -- placement uses an equal-weights Fitch proxy regardless of concavity/min_steps. The equal-weights-proxy placement contract was already documented on @PARAM concavity in a prior commit; this just fixes the remaining min_steps gap and adds regression coverage: a mock-capture test pinning that AdditionTree() forwards real min_steps to ts_wagner_tree, a direct ts_wagner_tree comparison showing min_steps changes the score but not the edge set, and a characterisation test locking in that numeric concavity never moves AdditionTree()'s topology. Also records, as a comment only, that wagner_goloboff_scores()'s tip_ambiguous computation ANDs across all n_states including the NA plane, so a tip coded {0,1} -- every applicable state but not "-" -- is scored non-ambiguous (informative) despite carrying no information about which applicable state holds. Left as-is: changing it shifts start trees and needs an A/B (T-371). Fixes #5 Fixes #6 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ps tests External review of the prior commit found the roxygen text factually wrong: AdditionTree() never returns a $score (it discards result$score), so "affects only the reported $score" is false -- correct it to state plainly that numeric concavity has no effect on AdditionTree()'s output. Also addresses two test gaps the review flagged: the first new test exercised ts_wagner_tree() directly rather than AdditionTree() (retitled to make that explicit, and its equality assertion tightened to a directional/finiteness check so a NaN or sign error couldn't pass), and two cheap regression tests were missing: that concavity = Inf still forwards min_steps = integer(0), and that concavity = "profile" still runs cleanly now min_steps is computed post-recoding. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Aug 4, 2026
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.
Fixes #5
Fixes #6
#5 (T-369)
AdditionTree()'s numericconcavitynever affected the returned topology — placement always uses an equal-weights Fitch proxy (fitch_indirect_length_cached), andconcavity/min_stepsonly enter the finalscore_tree()call. That contract was already documented in a prior commit on@param concavity; this PR:searchArgsnever passedmin_steps, so a finiteconcavityscored the internalresult$scoreagainstmin_steps = 0instead of the dataset's real per-character minimum (MinimumLength(dataset, compress = TRUE), matching the idiom already used inMaximizeParsimony()/Resample()/SuccessiveApproximations()).AdditionTree()discardsresult$score, so this has no user-visible effect — corrected purely for internal correctness and to stop future confusion.concavity"affects only the reported$score", butAdditionTree()never returns a score at all. Reworded to say numericconcavityhas no effect onAdditionTree()'s output, full stop.AdditionTree()forwards realmin_stepstots_wagner_tree(fails pre-fix, confirmed); a directts_wagner_treecomparison showingmin_stepschanges the score but never the edge set; a characterisation test locking in that numericconcavitynever movesAdditionTree()'s topology; and two small guards (thatconcavity = Infstill forwardsmin_steps = integer(0), and thatconcavity = "profile"still runs cleanly now thatmin_stepsis computed after profile recoding).Explicitly left undone (per the issue's own framing, a separate enhancement): using the weighted scorer for placement instead of the EW proxy. That would change start trees package-wide and needs its own wall-clock/reach A/B — out of scope here.
#6 (T-371)
Comment-only. Adds a note on
wagner_goloboff_scores()'stip_ambiguouscomputation (src/ts_wagner.cpp) recording that it ANDs across alln_statesincluding the NA/inapplicable plane, so a tip coded{0,1}(every applicable state but not-) is scored non-ambiguous/informative despite carrying no information about which applicable state holds.wagner_entropy_scores()'s weight-blindness comment matching Goloboff's was already present oncpp-searchfrom a prior pass, so nothing more was needed there.Explicitly left undone: changing either behaviour (weight-awareness of the entropy scorer, or treating
{0,1}as ambiguous). Both are judgement calls that shift start trees and need their own A/B — this PR only makes the existing behaviour legible.Validation
.o/.dllcleaned after everyroxygen2::roxygenise()/check_man()pass per the debug-contamination gotcha.tests/testthat/test-AdditionTree.R: 36/36 pass against a temp-library install (notload_all()).devtools::check_man(): clean.spelling::spell_check_package()and the exacttests/spelling.Rinvocation: clean.external-revieweragent pass, scoped to correctness of the documented contract and whethermin_stepsgenuinely leaves the topology untouched (verified structurally insrc/ts_wagner.cppand empirically across all 30inapplicable.phyDatadatasets: 60/60 topologies identical). Two findings from that review (the$scoredoc inaccuracy, and a mislabelled/soft test assertion) are fixed in the second commit.agent-check.yml) is currently failing on this branch, but for an unrelated, already-tracked reason: pak cannot resolve theMaxMinSuggests dependency (agent-check.ymlwindowsjob fails on every branch:MaxMinis not published in ms609.github.io/packages, so pak cannot resolve it #43,in-progress), reproduced identically oncpp-searchitself at the time of dispatch (run 30929069059, predating this PR's dispatch). Not caused by this change.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com