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

Remove the topology-only clock #73

Open
hyanwong opened this issue Dec 6, 2019 · 4 comments
Open

Remove the topology-only clock #73

hyanwong opened this issue Dec 6, 2019 · 4 comments

Comments

@hyanwong
Copy link
Member

hyanwong commented Dec 6, 2019

At the moment in the forwards_algorithm, the section labelled # Topology-only clock assigns into the vv variable, but then nothing is done with that variable. It looks like there's a mistake there to me.

@awohns
Copy link
Member

awohns commented Dec 6, 2019 via email

@hyanwong
Copy link
Member Author

hyanwong commented Dec 6, 2019

I guess you meant to do the same as the previous lines, and assign val *= vv and probably g_val *= vv too in that branch of the if statement?

@awohns
Copy link
Member

awohns commented Dec 6, 2019 via email

@hyanwong hyanwong changed the title Topology-only clock is either pointless or buggy Add unit tests for the topology-only clock Dec 10, 2019
@awohns awohns added this to the tsdate Version 1.0 milestone Nov 21, 2020
@hyanwong hyanwong changed the title Add unit tests for the topology-only clock Remove the topology-only clock Nov 5, 2024
@hyanwong
Copy link
Member Author

hyanwong commented Nov 5, 2024

Tests have show that basing node times on a naive expectation under the coalescent based on each marginal tree (what we call the "topology-only" clock) does not give good results (see e.g. #292), so I think we should simply remove the ability to do this, and raise a NotImplementedError

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

No branches or pull requests

2 participants