search

lamho86 / phylolm

GNU General Public License v2.0
30 stars 12 forks source link

Minor issue when using my own transformed tree. #35

Closed jboyko closed 3 years ago

jboyko commented 3 years ago

Dear Dr. Ho,

The input diagWeight was very low, but non-zero which meant it would be rejected based on this error check. However, if this error is bypassed with this minor change, the likelihoods match exactly what we find using typical inversion methods.

Best, James

cecileane commented 3 years ago

This change could be dangerous, if only checking that diagonal weights are != 0.0. Checking absolute equality, for floating point values, is not recommended in general. Could the criterion be made more lenient instead, like this (without the 0.8 power)?

any(abs(diagWeight) <= .Machine$double.eps)

With weights below the machine precision, there would no guarantee that the calculations are reliable.

jboyko commented 3 years ago

Thanks for your reply Dr. Ané! I didn't know about those issues, but that makes sense. Unfortunately, a lot of the diagonal weights I'm getting after the tree transformation are below machine precision. Please forgive my ignorance, but I had thought that since my diagonal weights were so low, they may be excluded. This assumption has proven completely false and I'm not really sure I understand exactly why. Would there be a way to include the diagonal weights as part of the tree transformation itself (such that I would avoid providing diagonal weights)?

jboyko commented 3 years ago

I've decided to rescale the tree to a max branching time of 1 prior to the tree transformation which will allow for a max alpha of -log(.Machine$double.eps^0.8) when using three.point.compute. Should be a more than sufficient max alpha. Thanks again for your comment!

lamho86 commented 3 years ago

Sorry for my late reply. It is good that you can fix your issue with rescaling the tree.