Default epsilon is occasionally too small

[nspope]

In edge cases (e.g. with a single EP iteration) the epsilon used to force positive branch lengths is too small, and gets lost due to floating point error, causing the subsequent tables.sort() to error out. In general, a "safe" lower bound for epsilon is going to depend on the mutational clock & time scale: that is, if the mutation rate is very low and/or population size very large then we might run into issues even outside edge cases.

It would be nice to (1) document the eps argument somewhere so it's more clear how to work around issues like this (and maybe rename it to be clearer; something like min_branch_length); (2) if eps falls below the minimum floating point difference for a particular edge length, use a larger value (but still small in relative terms) so that we don't error out.

[nspope]

Oh wait, I see eps is already in the docstring for the individual methods. (It'd be nice to rename this to 'min_branch_length' IMO.) But (2) would still be good to do.

[hyanwong]

Happy to rename it, and keep eps (which is a horrible name) as a deprecated equivalent. But does eps control other epsilon values.

For (2), as tsdate is so fast, would it be simpler to simply detect the error and bomb out saying "try again with a larger eps"?

[nspope]

Hmmm, I think for (2) the right thing would be to set the parent age to the next floating point up from the child age (if this is larger than eps); this shouldn't add any additional complexity and will keep a literal interpretation of "min_branch_length" accurate.

While tsdate itself is fast, one can easily imagine it being integrated in a bigger workflow for which bombing out might be a much bigger deal. (For example, if it's part of a snakemake workflow, a single job bombing out will halt the workflow). In general I think if there's a obvious fix to make an edge case work, we should take it.

[nspope]

Wrt other eps; oh I'd forget we're using the same argument for two distinct things-- and it seems like these should in fact be two separate arguments? The min_branch_length usage is shared across methods; any other usage is particular to inside-outside I assume? I don't think this dual usage is clear from the docstring.

[hyanwong]

Yeah, I guess that would be more sensible, if any need tweaking. Perhaps a reason to describe min_branch_length.

We could say eps is the low-level default for epsilon values, but can be (should be?) overridden for branch lengths if the more specific min_branch_length is specified.

[nspope]

Per hit this in #295 (comment) so maybe it's not such an edge case

[hyanwong]

It would be great if other people don't hit this.

I'm slightly nervous about making the parent age the next floating point up, as then we can't insert any intermediate nodes. But in general I'm happy with tweaking and renaming the eps settings. Have you got a specific proposal here @nspope ?

[nspope]

That's a good point. No, I haven't had time to think this through carefully, was hoping to deal with this (and a couple other tsdate things) over the winter break.

Another option would be to frame eps in terms of mutation rate, e.g. min_branch_length = eps * mu, and set eps to 1 by default.