-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add mixture-of-gammas global prior, updated by EM #349
Conversation
Wow, that's great. What other issues is this linked to? We still have to figure out if there are better ways to make the prior bespoke for each node, right (i.e. #292)? Do we need to decide on an API yet? Or should we keep this undocumented? It would be good to release 0.1.6 before Xmas if the changes in #346 are all OK. |
Currently getting this:
Maybe I need a more recent numba version (in which case we should specify a minimum version) Edit - confirmed that upgrading to numba 0.58.1 fixes it. |
This doesn't work, because of assuming the form of global prior when storing provenance. Using e.g. I presume we either store the value |
Fixed -- so numba 0.58.1 as a minimum version? |
I guess so. Can't hurt, right? |
I just did this in the mom-fixup branch. Yea, there's enough code movement in numba that pinning the version is a good idea regardless |
Pinning to recent numba is fine - I think in practise you end up with needing the latest version pretty quickly anyway, what with numpy updates. |
2aaf053
to
ced5d8b
Compare
This is ready ... I'll add support for user-specified priors in a later PR. I've checked that it works with a large (2k sample) mosquito tree sequence. It'd be great to make sure this runs through with global_prior set to 1 and then set to 5 on GEL or UKB, when you have the time, @hyanwong. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems good to me. I have made a version that works with the latest main branch (new API version) in main...hyanwong:tsdate:mix-prior-merge - feel free to steal stuff from that branch. I have set most of the defaults to None, then overridden then with a sensible default, which makes it easier to change the defaults later without causing an API change.
I just ran it on the GEL dataset and it went through without problems with global_prior set to 1 and then 5. Can't vouch for the accuracy, of course! |
More streamlined numerical checks Initialize gamma mixture from conditional coalescent prior Add pdf Update mixture.py to use natural parameterization WIP Moved fully into numba Cleanup Cleanup More debugging WIP Working wording Add missing constant to loglikelihood Skip prior update completely instead of components Skip prior update completely instead of components Remove verbose; use logweights in conditional posterior Move mixture initialization to function Docstrings and CLI Remove some debugging inserts Remove preemptive reference Fix tests
tsdate/core.py
Outdated
@@ -1682,7 +1790,8 @@ def variational_gamma( | |||
max_iterations=None, | |||
max_shape=None, | |||
match_central_moments=None, | |||
global_prior=True, | |||
prior_mixture_dim=1, | |||
em_iterations=10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think prior_mixture_dim and em_iteration defaults need to be None (which then get overridden below)
@@ -954,32 +955,37 @@ class ExpectationPropagation(InOutAlgorithms): | |||
Bayesian Inference" | |||
""" | |||
|
|||
def __init__(self, *args, **kwargs): | |||
def __init__(self, *args, global_prior, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I quite understand how the global prior is used differently from self.prior here. Is it worth adding a docstring to explain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, module two minor comments. I think I'm a bit confused about how the the global_prior differs from the "normal" self.prior, so maybe worth a minor bit of documentation?
self.priors.grid_data[:, 0], self.priors.grid_data[:, 1] | ||
) | ||
|
||
self.global_prior = mixture.initialize_mixture( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a comment here about the difference between self.global_prior and self.priors would be helpful? They sound like the same sort of thing.
done -- I think this is good to merge, but it'd be nice to check a few examples before putting a release out. I can do that tomorrow. |
Great, thanks. Merged. |
K
components, passprior_mixture_dim=K
em_iterations=0
prior_mixture_dim=1, em_iterations=0
prior_mixture_dim=1, em_iterations=10
)