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

Api extras #351

Merged
merged 4 commits into from
Jan 4, 2024
Merged

Api extras #351

merged 4 commits into from
Jan 4, 2024

Conversation

hyanwong
Copy link
Member

@hyanwong hyanwong commented Jan 4, 2024

Make tsdate.date() a wrapper for tsdate.inside_outside(), tsdate.variational_gamma(), or tsdate.maximization(). This allows easier documentation and reuse, and is a follow-up to #350

Also ban tsdate.maximization() from using the return_posteriors parameter.

Make tsdate.date a simple wrapper for functions such as tsdate.inside_outside, etc.
Copy link
Contributor

@nspope nspope left a 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
Copy link
Contributor

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

tsdate/core.py Outdated Show resolved Hide resolved
tsdate/core.py Outdated Show resolved Hide resolved
@nspope
Copy link
Contributor

nspope commented Jan 4, 2024

also what do you think about splitting the contents of core.py into different files? It's getting a bit big, what with the API, the algorithms, and the likelihood objects?

We don't have to do that now though (it'd be better to wait until the conflicts with #349 are resolved)

@hyanwong
Copy link
Member Author

hyanwong commented Jan 4, 2024

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 core.py, which we can do later. It may also be that we can place some of the gamma stuff within the VariationalGammaMethod class?

@hyanwong hyanwong merged commit 51f1b88 into tskit-dev:main Jan 4, 2024
3 checks passed
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