modify checks arg lik #2232
modify checks arg lik #2232
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2232 +/- ##
=======================================
Coverage 91.52% 91.52%
=======================================
Files 20 20
Lines 11334 11337 +3
Branches 2302 2304 +2
=======================================
+ Hits 10373 10376 +3
Misses 523 523
Partials 438 438
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
|
It has been two years since I've really looked at this, but I don't think multiple roots should cause any problems in the likelihood function implementation. Was the point of your update in #2107 that the existing test was incorrectly picking up multi-root trees as error cases @GertjanBisschop? If so, then I think the right fix is to just tweak the test to not do that, without catching ARGs with multiple roots with a new error message. Or am I missing something? |
|
I was mainly unsure about what happens when we add in nodes by decapitating for example. Then we have a bunch of floating lineages essentially not ending with a common ancestor event. What is the likelihood of such a lineage given that the likelihood computation, from my understanding, checks whether given a coalescence and recombination rate what the next out of those two events is, and what the likelihood is associated with that. |
|
You're right: floating lineages ending in a degree 1 root would cause problems. Roots with degree 2 caused by common ancestor events are already handled correctly. Is there a good way to detect that distinction? Edit: It would be pretty easy to add support for the situation where the ARG has just been deterministically stopped before all roots have been reached, causing all remaining extant lineages to end in degree 1 roots at exactly the same time. We'd just use the usual likelihood function until the last recombination or common ancestor node, and then include one more factor for the probability that nothing else happens until the end time. The only thing I'm not sure of is how easy it would be for the function to detect this case. If there can be floating lineages ending at a variety of times for no apparent reason, then I'm not sure what the interpretation would be and we'd probably want to just return an error. |
|
If I'm reading the new test right, then I think there is an issue we need to solve before merging. It looks like this likelihood will return a multiple roots error for any ARG with more than one root. That will include nearly all msprime realisations, even with Before merging, we should get this to a stage where multiple common ancestor roots don't throw an error. Else we're rendering the likelihood function effectively unusable. |
|
Yep was about to say the same. I agree with @JereKoskela. Modifying the computation to say that we stopped before another event happened is the way to go. I'll create an issue. |
|
This will return an error in the presence of any local tree with more than one root. This root does not need to be the same across local trees, so the graph does not need to have a single root. Or am I misunderstanding your comment? |
|
Ok, that sounds better. I was misunderstanding. In that case let's merge and continue in the issue you just opened. |
GertjanBisschop
force-pushed
the
arg-lik-error
branch
from
fa0cce1 to
cfe1d95
Compare
Slight modification to the polytomy check when computing likelihoods.
There are a couple of test cases in the current test suite that include examples with multiple roots (within a single local tree). Thus far these did not fail as they were always limited to two roots at most.
These checks could probably be a bit more sophisticated as there are many cases where a decapitated arg could easily be associated with a well-defined likelihood value. @JereKoskela, input welcome.