richelbilderbeek / pirouette_article

Article about pirouette, by Bilderbeek, Laudanno and Etienne
GNU General Public License v3.0
0 stars 0 forks source link

Feedback 2020-07-31 #119

Closed richelbilderbeek closed 4 years ago

richelbilderbeek commented 4 years ago
  1. I don't understand how the function create_exemplary_dd_tree can yield a tree with only 5 taxa, because it is using the default, which says it is 6 taxa. The code only selects trees with 6 tips, so this can't be stochasticity. So either you changed the default (but I downloaded both the master and develop version of pirouette and they still have 6 as default), or you hard-coded 5 in the function, i.e. create_exemplary_dd_tree(n_taxa = 5). But then this should be stated in the main text, and the supplementary material should not say that this is because of stochasticity.

The text is right: create_exemplary_dd_tree creates a distribution of trees with an expected number of 6 taxa. Here is the distribution:

Screenshot from 2020-07-29 16-10-46

The code to generate the trees is still in all priouette examples. The implementation of create_exemplary_dd_tree is already replaced by that code, but only on the richel branch.

1a. I also think that it is useful to use a seed in the function (or to report it), because otherwise the example cannot be replicated (I could not). I know there are issues with random seeds in case of multithreading, but I think multithreading is not an issue here.

We do use exactly the same seeds, which results in exactly the same results in the same environment (see https://github.com/richelbilderbeek/pirouette_article/issues/115). A different environment (operating system, R version, etc), however, gives a different random number sequence from a same seed. Due to this, one could argue, no research that draws at least one random number is never perfectly reproducible on all environments.

  1. You did not respond to my request that I feel that the pirouette default should be that the tree model used to simulate the twin tree is the same as the tree model used to infer the true tree. Only then, we are truly looking at the baseline error. This does not mean that we need to rerun everything, but only change the package and change the text wherever it says something about the default settings.

Indeed, I chose to ignore this, because it feels off-topic to me: we try to get an article submitted before a deadline. I suggest we discuss this after submission. I have broken pirouette in multiple places to make it to the deadline, I would enjoy first fixing that. It's also the reason you could not reproduce the exemplary tree :sunglasses: :+1:

  1. In several places in the supplementary material where you note that the twin tree was generated with a BD process but the (best) inference model was Yule, I added that the extinction rate used in generating the tree was practically 0 so the BD process resembles a Yule process.

Thanks!

richelbilderbeek commented 4 years ago

It's submitted! Thanks @Giappo!