Repository navigation
NNIDist(regionTips): exact-per-region upper bound - #225
Open
ms609-agent wants to merge 8 commits into
Open
ms609-agent wants to merge 8 commits into
ms609-agent wants to merge 8 commits into
Conversation
PR #28 (2020) summed diameter quantities into pairwise bounds. best_lower added the exact diameter of each region, which bounds a pair's distance from above; lower added a lower bound on the diameter, which exceeds the number of unmatched edges from 37 edges. Li et al. (1996) give only one pairwise lower bound: one NNI per unmatched edge. Both fields now report that count. fack_upper / NNIDiameter()'s fackMax now use Fack et al. (2002) theorem 3, which is never looser than the theorem 1 bound used before. Document all seven NNIDist() fields; drop the unused min_diameter table; add tests that every bound brackets the exact distance on all 6- and 7-leaf tree pairs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Opt-in `region_upper`: each region of unmatched edges with up to `regionTips` leaves contributes the exact NNI distance between its two subtrees (bidirectional BFS); other regions contribute best_upper's term. An upper bound only: NNI distance is not decomposable (Li et al. 1996). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Also fix a pre-existing test length mismatch (rep(NA, 7L) with six names). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # NEWS.md # R/tree_distance_nni.R # man/NNIDist.Rd # src/nni_distance.cpp # tests/testthat/test-tree_distance.R # tests/testthat/test-tree_distance_nni.R
This branch has not been deployed
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.
Adds an opt-in, tighter upper bound to
NNIDist(). This is the follow-up to #222 that was deferred there.What it does
NNIDist(tree1, tree2, regionTips = n)appends a seventh element,region_upper. Each region of unmatched edges with up tonleaves (n≤ 16) contributes the exact NNI distance between its two subtrees, found by bidirectional BFS over the region's tree space. Larger regions contribute the region'sbest_upperterm (its diameter or bound), as do regions with polytomies and searches that visit more than 2²¹ trees.Reconciling regions one at a time is a valid transformation, so
region_upperis an upper bound and is never abovebest_upper. It is not documented as exact. Li, Tromp & Zhang (1996), Lemma 3, show that a shortest path can break a split the two trees share.With
regionTips = 0L(the default), the output is unchanged.Implementation (
src/nni_exact.cpp).NNIExact(tree1, tree2, budget)returns the exact distance for trees of up to 16 leaves. It serves as the test oracle and is used by the benchmark.Validation
lower ≤ exact ≤ region_upper ≤ best_upperand symmetry hold on all 5,565 six-leaf pairs, all 446,985 seven-leaf pairs, and 311,850 eight-leaf pairs (30 random trees × all 10,395).region_upper == exactheld on every pair checked. That covers n ≤ 8, plus 270 near-identical pairs at n = 12–16. This is empirical: Li et al. note that Day's decomposability conjectures hold up to 7 leaves, and Lemma 3's counterexample needs large n. The 6/7-leaf testthat sweep asserts this equality as a regression check, against an independent R BFS.Benchmarks (
benchmark/nni-region-upper.R, release install, 1 core)Cost of one region with no shared splits (median / max seconds; share solved within budget):
Tightness against the exact distance at n = 7, 8 (means):
Larger trees (
regionTips = 16; means of 50 pairs, ms per pair):The bound pays off for similar trees, where regions stay small; there it nearly closes the gap to
lower. Unrelated trees form one large region, which gains nothing. At n = 12–16 they also pay the full search budget, about 2–4 s per pair, before falling back.Recommendation
regionTips = 0L. The docs say that up to 11 leaves costs under a second per region, and that larger regions pay off only when the subtrees are similar.NNI_EXACT_BUDGETstays at 2²¹, so most 12-leaf regions solve; a failed search costs about 4 s.Notes
mainafter NNIDist(): make lower bounds valid; adopt Fack theorem 3 #222 merged. The merge keptmain'sbest_upper-second ordering.🤖 Generated with Claude Code