Skip to content

Refactor species tree search - #5

Open
StefanFlaumberg wants to merge 3 commits into
noahares:mainfrom
StefanFlaumberg:main
Open

Refactor species tree search#5
StefanFlaumberg wants to merge 3 commits into
noahares:mainfrom
StefanFlaumberg:main

Conversation

@StefanFlaumberg

Copy link
Copy Markdown

This is a standalone pull request, it makes the following changes:

PLLRootedTree:
Implementation of both SpeciesTree::getHash() and SpeciesTree::getNodeIndexHash() functions is moved unchanged to the PLLRootedTree::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 any SpeciesTree-specific features.

SpeciesRootSearch:

  • Add the optimizeDates() for the initial tree at the beginning of rootSearch(). The rationale is that every new root position is evaluated for a date-optimized tree (see the optimizeDates() call in rootSearchAux()), so it is reasonable to start the search with a tree that is date-optimized as well.
  • Merge the two initial rootSearchAux() calls into one special four-way call. To this end, in rootSearchAux(), 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 to moveHistory outside the rootSearchAux(), i.e. before the inequality, which always increased movesHistory.size() by one. Note that this first added direction was ignored later in rootSearch in the loop across bestMovesHistory, so now it is just removed, thus making things much simpler.
    Also now the rootLikelihoods- and treePerFamLLVec-filling code blocks are needed only once, at the beginning of rootSearchAux(), so that both the initial and all the tested topologies reach them.
    These all are purely layout simplifications, the logic remains the same.
  • Suppress bestStackLL inheritance between siblings in rootSearchAux(). For example, the current branch has two child branches and the original bestStackLL, the loop first tests the root placement onto the first child and happens to find a better likelihood, this likelihood becomes the new bestStackLL for the subtree of the first child and recursion proceeds into this subtree, then the original loop tests the second child -- now, against the original bestStackLL, whereas before, against the new bestStackLL of 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 of bestStackLL as the likelihood of the best root on the path from the initial root to the current branch. It is worth noting that bestStackLL was 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 extend maxDepth) and less deep for large maxDepth (as switching earlly to the local search would constrain maxDepth). Both are probably good (as rootSearch(-1) rarely finds anything better at all, yet takes quite some time) and have little effect, but I haven't run thorough tests.
  • Everything else is cosmetic changes.

SpeciesTreeOperator:

  • The assert(canApplySPRMove()) check is added to applySPRMove().
  • The root is now also included by 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 of getPossiblePrunes() could be to select prunes based on clade quartet supports, but it is not used now.
  • Refactor getPossibleRegrafts() and recursiveGetNodes() to obtain the same ragrafts using less deep recursions and add a comment to explain what getPossibleRegrafts() 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...
  • Everything else is cosmetic changes.

SpeciesSearchCommon:
Only cosmetic changes. For instance, as veryLocalSearch() tests only a single input prune, there was no use running the across-prunes loop, which is now removed.

SpeciesSPRSeach:

  • Add the canApplySPRMove() check inside the across-regrafts loop. The rationale is that calling veryLocalSearch() 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).
  • Everything else is cosmetic changes.

SpeciesTransferSearch:

  • Make the round-stopping criterion consider only the tested transfer moves. The stop happens after 50 consecutive failures, but only if we've already gone through minTrials = max(50, speciesNumber / 2) transfer moves during this round. Before, we considered here all transfer moves, even those rejected by alreadyPruned.find() or canApplySPRMove(), counted via the index variable. Now, we use the trials variable 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 minTrials threshold has been passed anyway.
  • Fix the logic of MovesBlackList to make it actually work, but disable it for now to preserve the current behaviour. MoveBlackList is 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 transfers and support members of TransferMove, 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.
  • Make the TransferMove:operator<() actually mean "less" instead of "more". Accordingly, getSortedTransferList() now uses reverse iterators to sort transfer moves in descending order of their support. So this is purely a semantic change, the logic remains the same.
  • Everything else is cosmetic changes.

All Species*Search main functions:
Add the assert(evaluator.computeLikelihood() == searchState.bestLL); check at the beginning of rootSearch(), SPRSearch(), and transferSearch() to ensure that searchState is correctly updated throughout the run.

@StefanFlaumberg

Copy link
Copy Markdown
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 SpeciesRootSearch class and in the transferRound() function (using trials instead of index).

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,
Stefan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant