-
Notifications
You must be signed in to change notification settings - Fork 72
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
Better divmat site mode #2813
Better divmat site mode #2813
Conversation
I tried running this on the unified genealogy trees for chr5 q (hgdp_tgp_sgdp_high_cov_ancients_chr5_q.dated.trees). Details:
For one thread it needs 2.7G of RAM. Running with 8 threads needed 6G of RAM (reasonable, since we're creating 8 n*n matrices). The old stats API based code quickly ran out of memory on my machine (32G RAM). Finished up after about 3 hours:
|
73dc0cf
to
bba57c1
Compare
Ugggghhhh, looks like there's some memory access problem. Digging. |
ebb68bc
to
4945dd8
Compare
OK, fixed the memory error, ready for review! |
4945dd8
to
d9810bd
Compare
And here's an experimental variant-based method. Has basically the same performance (some wrinkles to be worked out with multi-allelics) |
I've updated this to include the correct algorithm for dealing with multi-allelic sites, to give the same answer as the stats API version. This will be easy to implement in C, if we decide to make the change. What do you think @petrelharp? Unless we can see a clear way forward for making an efficient version counting mutations on the paths, I'm not sure there's much point in creating confusion here by using the number-of-mutations version for site-mode. |
Just had a chat with @petrelharp and we decided to change the definition to exactly match the current stats API. |
8c2861e
to
ea6a53f
Compare
I think this is basically ready to go @petrelharp (although I need to rerun the benchmarks) |
ea6a53f
to
86e2960
Compare
LGTM! Things I have noticed are pending are
Also, I think we should add the |
Great, thanks @petrelharp! Good spots on all counts. I think we can merge this much for now, and follow up with more PRs. Looks like this version is a small bit slower than the last version (~20%) but I don't think that's worth worrying about. I logged issues to track the remaining stuff I think: |
86e2960
to
efc3313
Compare
Manually merging because we're missing a formal "approval" |
An improved mutation-by-mutation algorithm for site mode divmat. See #2779 for discussion
Benchmarks:
I tried running it for n=5000 also, but ran out of patience. I think it's clear that the new algorithm is a lot better for this regime anyway.
Code: