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

Remove variance branch length parameter #26

Merged
merged 1 commit into from
Jan 20, 2023

Conversation

theosanderson
Copy link
Owner

@theosanderson theosanderson commented Jan 20, 2023

This parameter muddies the waters. It was originally added to scale the prior distribution for the branch lengths, and initially (wrongly) played quite a significant role in assignment. We realised that this didn't make sense, and decided to either relax it or to remove it (#13). At that time we did some tuning experiments, comparing to TreeTime results, and found that a value of 10 performed a tiny fraction better than a value of 100 (but probably within random noise). At that time I decided to relax it to 10 rather than remove it entirely, based on that data. But upon receiving reviews for the paper I have decided to remove it both because

  • the existence of this parameter causes confusion, because it makes the inference less clear
  • it reduces the flexibility of the algorithm as it would exert a big effect if you tried to use Chronumental for data spanning centuries.

I have just some testing and shown that for the sort of datasets we're interested in it really makes almost no difference, and I'm sure in some cases things will work better without it.

@theosanderson theosanderson merged commit 793782a into master Jan 20, 2023
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