Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stack based refactor pt. 2 #34

Merged
merged 13 commits into from
Aug 21, 2024

Conversation

nossleinad
Copy link
Collaborator

Translate branchlength_optim! and nni_optim! from recursive to iterative

  • Use Iterators.reverse to avoid allocs and enforce a more consistent style in stack-based implementations
  • Don't ignore starting_index in set_node_indices

Note:
We need to decide if we want to go with nni_optim! or what I call nni_optim_full_traversal!. The latter ensures that all nodes are visisted during the optimization which the former does not.

@murrellb
Copy link
Member

murrellb commented Aug 5, 2024

We need to decide if we want to go with nni_optim! or what I call nni_optim_full_traversal!. The latter ensures that all nodes are visisted during the optimization which the former does not.

Do you have any side-by-side comparisons for tree search using both options? Try starting from random, vs starting from a nearby tree, etc?

@nossleinad
Copy link
Collaborator Author

Starting from a random tree
randomtree

Starting from a nearby tree
nearbytree

@nossleinad
Copy link
Collaborator Author

Full tree_polish! starting from a random tree
randomtree

Full tree_polish! starting from a nearby tree
nearbytree

@nossleinad nossleinad marked this pull request as ready for review August 21, 2024 10:32
@nossleinad nossleinad merged commit 0c4dc1a into MurrellGroup:main Aug 21, 2024
4 checks passed
@nossleinad nossleinad deleted the Stack-based-refactor-pt.-2 branch August 21, 2024 10:57
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.

2 participants