knausb / vcfR

Tools to work with variant call format files
248 stars 54 forks source link

adegenet::df2genind check.ploidy #106

Closed knausb closed 6 years ago

knausb commented 6 years ago

The adegenet function df2genind() is called by vcfR::vcfR2genind() to convert vcfR objects to genind. The function df2genind() has been changed to include the option check.ploidy which can improve its performance. This was documented here https://github.com/thibautjombart/adegenet/issues/231 and here https://github.com/thibautjombart/adegenet/pull/232. While this can be managed by setting an option, I feel that vcfR2genind() should include a parameter check.ploidy and mention this in its man page. The default should follow df2genind by setting its default to check.ploidy = getOption("adegenet.check.ploidy").

zkamvar commented 6 years ago

May I suggest adding the ... argument to df2genind() so users can add strata/population, etc as well as other options?

knausb commented 6 years ago

Yeah, actually that does sound like a better idea. The one thing I do not like about the ellipsis is that it does not make the users options explicit. But your idea will provide more flexibility. And it avoids the situation where I have to wait for adegenet to be updated on CRAN before I make the change. And I can add to the documentation to check ??adegenet::df2genind(). Thanks @zkamvar!

knausb commented 6 years ago

On master at 20ef8ec3112e4d95a3317a9b23608e258174fccf. Thanks @zkamvar, thanks @fdchevalier!

zkamvar commented 6 years ago

Looks good! A couple of things: users only need to search with one question mark otherwise, the search process takes longer than necessary. Also, you can actually link directly to the df2genind function with \code{\link[=adegenet]{df2genind}{adegenet::df2genind()}}

Sent from my iPhone

On May 24, 2018, at 12:52, Brian Knaus notifications@github.com wrote:

On master at 20ef8ec. Thanks @zkamvar, thanks @fdchevalier!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

knausb commented 6 years ago

I've had problems with links in the past. I think the issue was when you try to link an uninstalled package. This made me realize that adegenet was a Suggests and not an Imports. So I changed this. But I still think that the help system is a little more bullet proof. And it should work if you're not using RStudio. Now on the master branch at: e2e410d30d3252e263d206018d1de22d8195e56d.

zkamvar commented 6 years ago

I've had problems with links in the past. I think the issue was when you try to link an uninstalled package. This made me realize that adegenet was a Suggests and not an Imports. So I changed this.

I forgot that you didn't have adegenet in imports. In that case, keeping the help method definitely is better. In fact, if it's just for the sake of being able to link to that one function's documentation, there's no reason to add it to your imports considering adegenet is a pretty dependency-heavy package (no thanks to myself). It will only serve to increase the download time for first-time users.

knausb commented 6 years ago

Back to Suggests at: 9ef130933cef50e3391af130144f25838f254d88.