Repository navigation
NNIDist(): make lower bounds valid; adopt Fack theorem 3 - #222
Open
ms609-agent wants to merge 2 commits into
Open
ms609-agent wants to merge 2 commits into
ms609-agent wants to merge 2 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>
Performance benchmark results
|
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
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.
Fixes #220.
Why the bound was wrong
The error came from our own code in #28 (2020), not from the literature. Li et al. (1996) and Fack et al. (2002) bound the diameter of NNI tree space: the largest distance between any two trees with n leaves. #28 added these diameter values into a pairwise distance:
best_loweraddedexact_diameterfor each region. A diameter bounds one pair's distance from above.loweraddedmin_diameter = max(k, ⌈lg(N!)/4⌉), a lower bound on the diameter. It equals k (the number of unmatched edges) only for k < 37.The only pairwise lower bound in either paper is one NNI per unmatched edge (Li et al. 1996, §4). That was what
lowercomputed before #28. Li et al.'s Lemma 3 also rules out building a lower bound from exact per-region distances, because optimal paths can touch matched edges.Changes
lowerandbest_lowerboth count unmatched edges, so they are now equal.best_loweris kept for compatibility.fack_upper/NNIDiameter()$fackMaxuse Fack et al. (2002) theorem 3. It is never looser than the theorem 1 bound used before; for example, an 8-leaf region gives 17 instead of 18, and a 20-leaf region gives 85 instead of 98. This tightensloose_upperandbest_upper.NNIDist()fields and the reasoning behind the bounds. This also fixes the brokenfackMaxformula in the Rd.min_diametertable. Correct the provenance ofexact_diameter: the 12-leaf value comes from Fack et al. (2002), table 2.skip_on_cran()(about 70 s).test-tree_distance_nni.Randtest-tree_distance.Rpass locally;spell_check_package()is clean.🤖 Generated with Claude Code