liamrevell / phytools

GNU General Public License v3.0
207 stars 56 forks source link

make.simmap(): Check shape of Q matrix. And others. #10

Closed J-Moravec closed 7 years ago

J-Moravec commented 7 years ago

Hey Liam, nice package, fill nicely niche simmap created when it remained to be Mac only GUI only software:)

However, I have a few minor problems: By definition, rows of Q should sum to zero. It would be nice if make.simmap checked this and reported error. I am surprised that I can do t(Q) and still get results. Even when it sometimes (but not always) throw some warning or error.

Also, it would be nice if this was commented a bit better function description. The description in arguments is very convoluted, it would be better to write something shorter and then "see more in details" and here describe all possible arguments. This way, it is big block of text, where one can see all description and it is not clear if the description is for MCMC, estimated, empirical, fixed or vQ or pi and so on.

Also, I don't want to sound too negative, but variable names like AA, XX, ZZ are quite bad:/

And another question, this is kind of R question in general. It seems that it is "traditional" in R, that functions do a lot of stuff at the same time. Personally, I would divide each function into several logical steps implemented separately. It can be then unit-tested much more easily and, someday in future, C/C++ implementation of critical parts could be added (and if large-scale simulations like in simmap are to be done, R is unable to handle this)

Thanks, I hope that I was not too negative. Jirka

liamrevell commented 7 years ago

Hi Jirka. Thanks for your comments. phytools is a work in progress. Since this does not represent a bug (per se), I'm closing this issue now.