Refactor species tree search - #5
Open
StefanFlaumberg wants to merge 3 commits into
Open
Conversation
…st and disable it for now
…ance from siblings in rootSearchAux()
Author
|
Hi @noahares, While the description above may look intimidating, the changes are mostly just simplifications of the existing algorithms. The only real modifications are in the If anything seems off here or you can think of better solutions, I'll be happy to have a discussion! It is not necessary to update AleRax's submodule after accepting this, as I will then shortly add another PR here that will have a PR to AleRax as a dependency (so the AleRax update can be done already after accepting those two). Best regards, |
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.
This is a standalone pull request, it makes the following changes:
PLLRootedTree:
Implementation of both
SpeciesTree::getHash()andSpeciesTree::getNodeIndexHash()functions is moved unchanged to thePLLRootedTree::getTreeHash(bool useLeafHash)function and the static functions that it uses.The rationale is the symmetry with
PLLUnrootedTree, which also has its own hashing functions, and that the moved functions did not use anySpeciesTree-specific features.SpeciesRootSearch:
optimizeDates()for the initial tree at the beginning ofrootSearch(). The rationale is that every new root position is evaluated for a date-optimized tree (see theoptimizeDates()call inrootSearchAux()), so it is reasonable to start the search with a tree that is date-optimized as well.rootSearchAux()calls into one special four-way call. To this end, inrootSearchAux(), move directions are added in a smart way to allow for both the initial four-way and the following two-way recursions, the inequality in the return condition becomes non-strict:movesHistory.size() >= maxDepth. The latter is because the original version had the first direction added tomoveHistoryoutside therootSearchAux(), i.e. before the inequality, which always increasedmovesHistory.size()by one. Note that this first added direction was ignored later inrootSearchin the loop acrossbestMovesHistory, so now it is just removed, thus making things much simpler.Also now the
rootLikelihoods- andtreePerFamLLVec-filling code blocks are needed only once, at the beginning ofrootSearchAux(), so that both the initial and all the tested topologies reach them.These all are purely layout simplifications, the logic remains the same.
bestStackLLinheritance between siblings inrootSearchAux(). For example, the current branch has two child branches and the originalbestStackLL, the loop first tests the root placement onto the first child and happens to find a better likelihood, this likelihood becomes the newbestStackLLfor the subtree of the first child and recursion proceeds into this subtree, then the original loop tests the second child -- now, against the originalbestStackLL, whereas before, against the newbestStackLLof the first child (not even of its subtree, but of the child itself!). This new behaviour makes much more sense and likely reflects the original concept ofbestStackLLas the likelihood of the best root on the path from the initial root to the current branch. It is worth noting thatbestStackLLwas never inherited between the subtrees of the initial root, so now it is just also not inherited between the subtrees of any tested root.This is a slight algorithmic change. As siblings are now more likely to switch to the local search, this can make traversal a bit more deep for small
maxDepth(as switching to the local search could extendmaxDepth) and less deep for largemaxDepth(as switching earlly to the local search would constrainmaxDepth). Both are probably good (asrootSearch(-1)rarely finds anything better at all, yet takes quite some time) and have little effect, but I haven't run thorough tests.SpeciesTreeOperator:
assert(canApplySPRMove())check is added toapplySPRMove().getPossiblePrunes(). While the only node that cannot be pruned is the current root, the possible prunes are batch-inferred at the beginning of a round, so the current root can change, making the initial root a valid prune candidate, while invalidating some other inferred possible prunes. The real purpose ofgetPossiblePrunes()could be to select prunes based on clade quartet supports, but it is not used now.getPossibleRegrafts()andrecursiveGetNodes()to obtain the sameragraftsusing less deep recursions and add a comment to explain whatgetPossibleRegrafts()actually does because it uses a non-standard notion of SPR radius. The logic remains the same.In fact, now we always get regrafts for something between standard SPR radius n and radius n+1. It might be reasonable instead to make the functions use the standard notion of radius n (but still excluding GrandFather and Brother's children to avoid repeated moves) and make n = 2 the default radius in AleRax. This will add 3 more regrafts per prune (GrandUncle and Uncle's children). But tests would be needed to show if it increases accuracy...
SpeciesSearchCommon:
Only cosmetic changes. For instance, as
veryLocalSearch()tests only a single input prune, there was no use running the across-prunesloop, which is now removed.SpeciesSPRSeach:
canApplySPRMove()check inside the across-regraftsloop. The rationale is that callingveryLocalSearch()can change the tree topology in a way that makes one of the inferred possible regrafts to be the prune's brother. It was working fine without the check before because a SPR move to Brother is technically possible (changes nothing).SpeciesTransferSearch:
50consecutive failures, but only if we've already gone throughminTrials = max(50, speciesNumber / 2)transfer moves during this round. Before, we considered here all transfer moves, even those rejected byalreadyPruned.find()orcanApplySPRMove(), counted via theindexvariable. Now, we use thetrialsvariable here, which instead counts only the tested moves. Thus, the moves that have been invalidated during the round do not affect the stopping criterion anymore.This simplifies the logic and makes the search possibly a bit more exhaustive for large trees, as more moves have to be tested before the 50-consecutive-failure rule can be applied. In most real cases, however, the behaviour should not change at all, because the 50 consecutive failures are likely to happen either early in the round, when few moves have yet been applied, and thus new moves are unlikely too be invalidated before testing, or much later, when many moves have already been tested and the
minTrialsthreshold has been passed anyway.MovesBlackListto make it actually work, but disable it for now to preserve the current behaviour.MoveBlackListis intended to exclude from a new round the transfer moves that have already been tested in previous rounds. However, the original code had a clear inconsistency bug: the moves were added to the blacklist with the relative number of transfers, i.e. actual number / potential number of transfers, (line 79), but were searched for in the blacklist based on the actual number of transfers (lines 68,78) -- this way a blacklisted move could never be found in the blacklist, so the whole idea didn't worked.But how should things be then? I think that using the relative number of transfers would likely make blacklisting practically non-functional, as the potential numbers of transfers are likely to change between rounds very easily, making all the candidate moves looking "new" when being searched for in the blacklist. So instead, I opt to use the actual number of transfers for blacklisting the moves and the relative number of transfers for sorting the moves (via
transfersandsupportmembers ofTransferMove, respectively). This should filter out the moves that had the same transferred copies from the same regraft to the same prune in previous rounds. However, since the blacklisting technique has never actually been tested (due to the aforementioned bug), I disable it for now to preserve the original behaviour. It will be great to test its speed and accuracy effects in the future on large enough data.TransferMove:operator<()actually mean "less" instead of "more". Accordingly,getSortedTransferList()now uses reverse iterators to sort transfer moves in descending order of theirsupport. So this is purely a semantic change, the logic remains the same.All Species*Search main functions:
Add the
assert(evaluator.computeLikelihood() == searchState.bestLL);check at the beginning ofrootSearch(),SPRSearch(), andtransferSearch()to ensure thatsearchStateis correctly updated throughout the run.