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

Better timeslices #233

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

hyanwong
Copy link
Member

@hyanwong hyanwong commented Jan 9, 2023

Finally fixes #7 and produces a much nicer looking fit for large tree sequences, but leads to slightly worse performance for tiny tree sequences such as those tested in test_accuracy.py, because of #230. When we fix that, this PR should provide uniformly better performance, I hope

@hyanwong
Copy link
Member Author

hyanwong commented Jan 9, 2023

E.g. look at the oldest points in the before and after plots:

image
image

@awohns
Copy link
Member

awohns commented Jan 9, 2023

This is super @hyanwong!!
How about some simple metrics like MSE and spearman's to benchmark?

@hyanwong
Copy link
Member Author

hyanwong commented Jan 9, 2023

This is super @hyanwong!!
How about some simple metrics like MSE and spearman's to benchmark?

Yes, I could add MSE / Spearmans to the "unit" tests at https://github.com/tskit-dev/tsdate/blob/main/tests/test_accuracy.py I guess. The tests there are just to check that we don't accidentally introduce worse performance.

@hyanwong
Copy link
Member Author

hyanwong commented Jan 9, 2023

I added Spearman's in #236 @awohns (along with a test that we scale correctly with Ne)- do you want to check it looks OK and then I can merge #236 which will give us a decent backbone for testing changes such as in this PR?

@hyanwong
Copy link
Member Author

do you want to check it looks OK

Never mind, I merged it anyway, as it's only in the test suite.

@hyanwong
Copy link
Member Author

I think we should merge this anyway: looks like we might move away from a grid of timepoints to the variational method, but I don't want the fixed grid method to be completely forgotten about and consigned to the bin because of a timeslice issue.

Finally fixes tskit-dev#7 and produces a much nicer looking fit for large tree sequences, but leads to slightly worse performance for tiny tree sequences such as those tested in test_accuracy.py, because of tskit-dev#230. When we fix that, this PR should provide uniformly better performance, I hope
@hyanwong
Copy link
Member Author

Annoyingly this does seem to make accuracy worse in simple cases. Perhaps we should think about fixing the prior first, then return to this.

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.

Extend the top end of the adaptive time grid to include large values
2 participants