Feedback

[tsladecek]

1. Purpose of the package

The purpose of the package is short and clear. I do not know about other packages that provide something like this, so I can not really compare it.

2. Completeness

• I feel like there are too many functions :D. I think that some of them could be methods of one of the classes. More in the section below.

3. Code quality and sophistication

• No problem with the installation.

• it would be nice if there were some default arguments (for example for the popType in simDNAseq).

• It would make the review a bit easier if related functions were in the same file. Eg. all the SimBranch stuff.

• Maybe SFS can have its own class and you can add a plot method to it.

  • and maybe also a estimators method? Or just one summary that would contain all the statics and also Tajimas D

• Or you can add methods snp, sfs and estimators to the simDNAseq class? This way there are too many functions, and I feel a bit lost.

4. Documentation

• I was not able to access the vignette from the help page. We had the same problem. One solution is to specify that you want install them during the install of the package: devtools::install(build_vignettes = TRUE). This worked in our case. I

• I am not sure if having 2 vignettes is a good idea. I would prefer having just one, even if it meant that it is longer. I think you can connect them easily, eg. by showing how to simulate the 'segregating sites matrix', then calculating the SFS and finally the estimators.

• I like the vignettes and help pages, they are short and up to point.

• You use the word mutation rate a lot in both help pages and vignette, which can be confusing because you are talking about $\theta = 2N_e\mu$ where $\mu$ is also called mutation rate. And that is actually what I first thought when I saw that I should input value for mutation rate. Maybe just specify that you are talking about $\theta$ and not $\mu$.

• It would be more helpful to see the SFS plot (instead of the matrices with lots of zeros) for the normal population size, variable pop size and expanding. There should be some differences in the shapes of the SFS plots.

• for consideration In the documentation of your functions (in the example section), you can show how to use the output of different functions as an input to others. eg. for SFS segs = simDNAseq(10, 15, 10, 'sudExpPop', expansionTime = 2, proportion = 0.9) SFS(segs)

5. Interface

• The arguments are OK, everything is well explained in the help page. I did not have any troubles understanding what was going on, especially after reading the vignette.

• The simDNAseq could use some care when the parameters are not specified correctly. I don think you should make tons of unique checks and different error messages, maybe just check whether the parameters are of the correct type, pop_Type is of one of the valid entries etc. If not, you can create a generic error message that tells the user what should be the type of each variable, and maybe an example. I dont know, it might be too much, but we used that for a few programs that we were making for another class, and I think its nice to have program that doesnt shout at you everytime you make a mistake but rather suggests a solution :).

• BugI was also able to use the pairwDiff, mut_rate and TajimaD on the output of simDNAseq functions, without transforming it to SFS - I was not warned by anything.

7. Conclusion

The package delivers what it says. I like the documentation and the vignette, even though I think that the vignettes could be joined together. Some of the functions can be used as methods of other classes, or can be used to compose the summary of a class. Overall the package is ok. You should probably spend some time making sure that the input to your functions is correct. And also think about some idiotic moves such as I did, when I used the matrix of segregating sites as an input to the pairwDiff function.

[tsladecek]

Ok, now I see that it was me who did not say build_vignettes = True. Sorry :D

[UnicRain]

Thank you for the feedback. We agree that iit would be nice to add summary method and make warnings when SFS is not a vector. But we don't quite understand the last point under code quality and sophistication. We also agree that iit should be specified that $\theta$ is not the actual mutation rate.

There are other things we do not quite agree on, but we appreciate the feedback nonetheless.