nLTT statistic does not take stem into account

[richelbilderbeek]

From the vignette trees_with_root_edge.Rmd:

@thijsjanzen : do you agree this behavior is incorrect? If yes, I volunteer to fix it.

[thijsjanzen]

This behaviour is incorrect, but I'm unsure what types of trees p and q are?

On 12/09/16 14:48, Richel Bilderbeek wrote:

From the vignette |trees_with_root_edge.Rmd|:

issue https://cloud.githubusercontent.com/assets/2098230/18436268/c9150618-78f7-11e6-890d-1eda7b67b3f9.png

@thijsjanzen https://github.com/thijsjanzen : do you agree this behavior is incorrect? If yes, I volunteer to fix it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/richelbilderbeek/nLTT/issues/9, or mute the thread https://github.com/notifications/unsubscribe-auth/ASlXyHMCI87SKsQlVT6cQ6MC9tFlFHHdks5qpUoXgaJpZM4J6i8-.

[richelbilderbeek]

This is how they are created:

set.seed(42)
p <- ape::rcoal(4)
q <- p
q$root.edge <- 5

[thijsjanzen]

Ah! So the plot is incorrect, and not the nLTT statistic, assuming that the nLTT statistic assumes the crown age.

Perhaps we should add a couple of checks, such that the two trees that are compared are equally rooted (such that the user can select whether he is comparing rooted trees)

On 12/09/16 14:52, Richel Bilderbeek wrote:

This is how they are created:

|set.seed(42) p <- ape::rcoal(4) q <- p q$root.edge <- 5 |

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/richelbilderbeek/nLTT/issues/9#issuecomment-246337387, or mute the thread https://github.com/notifications/unsubscribe-auth/ASlXyJEY4fAEhpFCqkAOozx8l_XA6RDFks5qpUr6gaJpZM4J6i8-.

[richelbilderbeek]

I think the plot is correct, as I think it is natural to assume that a tree without a root == a tree with a root length of zero. I think the nLTT statistic should take the root lengths into account, unless you disagree (you are the authority here :sunglasses: ).

[richelbilderbeek]

One backwards-compatible fix may be to define nLTTstat_exact with an extra argument from as such:

nLTTstat_exact(p, q, from = "crown")

where from may also be root.

This does not have my preference, as I cannot see why the nLTT statistic would ignore stems. But again, you are the authority here :rainbow:

[thijsjanzen]

You are right, the plot is correct, and the nLTT function is incorrect. Off the top of my head I'm not entirely sure how to change the code to incorporate the root - the branching times function of ape doesn't have any extra flags... we could try and add the root time to the branching times vector?

I like the idea of an extra from argument, as this also makes it for the user more explicit what is going on.

[richelbilderbeek]

Let me fix that function, you'll see :sunglasses:

I am not a fan of adding the extra argument, as it just makes the natural assumption ('stems are also tracked') explicit, at the cost of making the user interface more complex. OTOH, I am not impressed by my own reasoning myself and I think both options (adding an extra argument yes/no) are close to equally good.

But, unless you change your mind (that there need not be an extra function argument), I will fix this tomorrow, with the extra argument.