-
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
Api extras #351
Api extras #351
Conversation
Make tsdate.date a simple wrapper for functions such as tsdate.inside_outside, etc.
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 -- this'll break #349 but it should be easy enough to fix. I worry a little bit about tying the output to a Results
class, because I think we may want to return different things from different methods ultimately (for example, the variational gamma method gives a per-edge "likelihood" that could be useful, and I'm not sure there's an analogue for inside-outside). But, if it comes to that I suppose we can make method-specific Results
classes that at a minimum contain the 4 fields in the base case.
@@ -6,7 +6,7 @@ tqdm | |||
daiquiri | |||
msprime>=1.0.0 | |||
scipy | |||
numba>=0.58.0 | |||
numba>=0.58.1 |
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.
Also need to update numba version in setup.cfg, looks like pip prioritizes this
also what do you think about splitting the contents of We don't have to do that now though (it'd be better to wait until the conflicts with #349 are resolved) |
Thanks so much for looking at this @nspope. Re the "results" class, this is an internal thing that is never exposed to the user (it's just to make out lives easier), so we can rework it later however we like. I agree about splitting up |
Make
tsdate.date()
a wrapper fortsdate.inside_outside()
,tsdate.variational_gamma()
, ortsdate.maximization()
. This allows easier documentation and reuse, and is a follow-up to #350Also ban
tsdate.maximization()
from using thereturn_posteriors
parameter.