How to allow unary nodes in variational_gamma

[hyanwong]

We probably want to allow advanced users to date using variational_gamma with tree seqs with unary nodes, but we want to fail or warn the casual user that this could be bad. What should the API for this be? In the previous methods we did it by doing the check when building a default prior, but we don't do this for variational gamma.

One possibility, which could be useful in the longer term, is to allow for a prior object that specifies the improper prior somehow. Then we can allow the variational gamma method to take a prior object (but it would only currently allow the improper prior to be specified). Then we could do the same as in the other methods: construction of the prior object could have an "allow_unary" parameter, for expert use.

[nspope]

I don't think we want a prior object (there is a prior used internally, on the roots, and I don't want to expose that to the user at all) -- and even if we had it I don't think that's the place for a unary node check. I can easily add a check for unary nodes to the mutation-harvesting routine, as this does a tree traversal in numba anyway. We can have a flag that disables the check.

[nspope]

... to clarify, I really don't want to complicate the API with custom input classes and unnecessary flexibility. It's in a good place right now -- date(ts, mutation_rate) -- and I think we should aim to keep it this simple.

[nspope]

also, I don't think there's a need to support unary nodes until there's a way to map mutations accurately between unary nodes (which is very likely not possible). Otherwise the dating algorithm will return rubbish, and we shouldn't bother making this a choice.

While the algorithm itself could be modified to do updates when mutations aren't mapped correctly around unary nodes, I don't think that's going to give much of an increase in accuracy at all (and it's a super low priority).

[hyanwong]

Yes, we can probably punt this down the road. I think that dating with unary nodes will probably be useful in very recent times (and for high recombination rates), but we're not in that regime yet anyway.

However, I do think that eventually we could make some progress on taking account of a better coalescent-ish prior. It's low priority, however.

[hyanwong]

date(ts, mutation_rate)

By the way, I changed this today so that mutation_rate was keyword-only. Is that OK with you?

[nspope]

Yup sounds good!

[hyanwong]

It's useful to be able to "date" recombination nodes in sc2ts, which are unary, so a (possibly undocumented) option to bypass the unary node check would be useful.