getNumberOfTrees: report the support, not the graph - #17
Open
alexeid wants to merge 1 commit into
Open
Conversation
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.
ITreeDistribution.getNumberOfTrees()was documented only as "the number of trees (topologies) in this distribution", which is ambiguous between the CCD graph and the distribution's support. They coincide for CCD0/CCD1 but not for full-support models.KRegCCD and MRegCCD both override
containsTreeto return true, yet inherited the graph-based count fromAbstractCCD. On a 129-taxon posterior KRegCCD reported log 88 against a true support of log 582, contradicting its owncontainsTree.numberOfRootedTopologiesandlogBigIntegertoAbstractCCDso all models share them;UniformEscapeCCD.numberOfRootedTopologiesstill delegateslogBigIntegerinCredibleCCDComputerthat was incorrect (new BigDecimal(bigInteger).scale()is always 0, so it reduced toMath.log(val.doubleValue()))NumberOfTreesContractTestNote the double-overflow boundary is ~155 taxa: 129 taxa needs 840 bits and converts fine, 160 needs 1093 and overflows. Coal160/Coal320/Yule200/Yule400 are past it.
Existing callers (
NNICladeExpansion,RogueDetection) use CCD0/CCD1 where graph and support coincide, so are unaffected. Full suite: 110 tests, 0 failures.