NESCent / popgenInfo

Vignettes for Population Genetics in R
http://popgen.nescent.org
MIT License
20 stars 50 forks source link

started LMM Genetic Diversity vignette and added data #182

Closed panastheod closed 8 years ago

panastheod commented 8 years ago

I copied the template to the use folder and placed the data in the data folder.

sessionInfo: R version 3.2.2 (2015-08-14) Platform: x86_64-w64-mingw32/x64 (64-bit) Running under: Windows 7 x64 (build 7601) Service Pack 1

locale: [1] LC_COLLATE=English_United States.1252 LC_CTYPE=English_United States.1252
[3] LC_MONETARY=English_United States.1252 LC_NUMERIC=C
[5] LC_TIME=English_United States.1252

attached base packages: [1] stats graphics grDevices utils datasets methods base

other attached packages: [1] git2r_0.14.0

loaded via a namespace (and not attached): [1] tools_3.2.2

hlapp commented 8 years ago

The tests fail, but (at least for now) on an (presumably) unrelated and pre-existing file:

Quitting from lines 99-126 (2016-01-26-SNP-selection.Rmd) 
Error in pcadapt(data = genotype, K = K) : object 'res' not found
Calls: <Anonymous> ... withCallingHandlers -> withVisible -> eval -> eval -> pcadapt

The container in which the tests are run was triggered to rebuild a week ago, so I suspect we have run into changes on CRAN. Perhaps the parameter interface of pcadapt() has changed? @zkamvar when you have a chance, can you look into this?

For confirmation, I have also triggered a rebuild of the test on master, and sure enough it now fails, too.

@panastheod if this requires a fix to files other than yours (which I suspect it will), are you comfortable enough with Git to cherry-pick commits or to rebase? If not, can you say how you are using Git (e.g., through a GUI, or through git2r, or some other way), so we can come up with instructions for you once we have a fix.

@zkamvar we should think about a way to automatically trigger a retest of master whenever the container on Docker Hub gets rebuilt. That should flag and catch such issues early on. I'll see whether we can use web hooks for this purpose.

zkamvar commented 8 years ago

The package PCAdapt has incurred breaking changes with version 3.0. I will contact the maintainer and the vignette author to see how this can be fixed.

zkamvar commented 8 years ago

This will additionally need the following packages:

library("lme4")
library("MuMIn")
library("multcomp")
panastheod commented 8 years ago

I am using git2r so let me know how i can i fix the issue when you have a solution. Its my first time using git so i will need instructions on how to do it. should i include in the vignette installing instructions for the packages we used?

Panas

On Wed, May 4, 2016 at 7:47 PM, Zhian N. Kamvar notifications@github.com wrote:

This will additionally need the following packages:

library("lme4") library("MuMIn") library("multcomp")

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/NESCent/popgenInfo/pull/182#issuecomment-216945655

zkamvar commented 8 years ago

I am using git2r so let me know how i can i fix the issue when you have a solution. Its my first time using git so i will need instructions on how to do it.

We will indeed let you know when #185 gets merged. Thank you for your patience.

should i include in the vignette installing instructions for the packages we used?

Not installation instructions, per se, but a short description of what the packages do.

hlapp commented 8 years ago

185 merged and master builds successfully again.

zkamvar commented 8 years ago

Hi @panastheod,

Now that #185 has been merged into the main repository, you can update your branch from upstream. This will be done by merging the master branch from upstream into your current branch.

It is important to note that you do not have to do this every time you want to make a change on an existing branch, this is a special case.

Here's how you can update your branch (assuming you've been following http://popgen.nescent.org/CONTRIBUTING_WITH_GIT2R.html):

library('git2r')
repo <- repository()
checkout(repo, "02-05-2016-LMM-Genetic-Diversity")
repo

At this point, you should check to make sure that the last line of the output says something like this

Head:     [bb045fd] 2016-05-02: started LMM Genetic Diversity vignette and added data

If this is the output you receive, then you can proceed with the following:

fetch(repo, name = "upstream")
merge(repo, "upstream/master")
push(repo, credentials = cred_token())

Once you've done this, your branch should be up to date :)

hlapp commented 8 years ago

@panastheod there is a test failure in your vignette:

Error in parse_block(g[-1], g[1], params.src) : 
  duplicate label 'load_data_show'
Calls: <Anonymous> ... process_file -> split_file -> lapply -> FUN -> parse_block
Execution halted

It looks like you are using the same label for several code blocks. load_data_show is one of the repeated labels, but I can see at least one other that's repeated too. Could you fix that?

zkamvar commented 8 years ago

It looks like you are using the same label for several code blocks. load_data_show is one of the repeated labels, but I can see at least one other that's repeated too. Could you fix that?

After looking at the vignette, this is only part of the pathology. I will go through and comment on specific issues.

zkamvar commented 8 years ago

Hi @panastheod, sorry for not responding earlier. This service only notifies us when you make a comment (since you can make several rounds of changes before it's ready for review). Thank you for making the critical changes. It's currently failing, but that's because we did not have the packages you are using in our dockerfile. We are currently rebuilding the dockerfile and once it's built, we'll ask you to make a small change and push again to make sure the vignette builds. Please stand by. Thank you for your patience.

hlapp commented 8 years ago

We are currently rebuilding the dockerfile and once it's built, we'll ask you to make a small change and push again to make sure the vignette builds.

That's not necessary. I'll relaunch the build once the Docker image is rebuilt.

hlapp commented 8 years ago

@zkamvar and @mmlopezu the build passes now.

zkamvar commented 8 years ago

Hi @panastheod,

Thank you for your patience.

One thing that would be nice is an explanation of the results. In your vignette, you do a good job explaining why you are using certain analyses, but the results are not explained/interpreted. Is it possible to provide this?

Additionally, there are small formatting issues, but these can easily be fixed on our end.

When you have made the requested changes, please respond to this thread.

Thank you, Zhian

zkamvar commented 8 years ago

@mmlopezu, do you have any comments on the vignette?

mmlopezu commented 8 years ago

Hi @panastheod

Here are some comments.

I think the vignette is great, but it would be great to have more explanation on some points:

Line 7 - In what cases would one want to compare genetic diversity between populations? I think it would be nice to give some real life examples (e.g. conservation questions, managed species, etc)

Line 9 - Can you explicitly say how you control for the variability of markers in ANCOVA and LMM?

Line 62 - Similar comment. Why is management fixed, and locus random?

Line 22 - I think you should say that the question is this study was whether or not "management" affected the genetic diversity of these populations. Readers would get a better feeling about why one could be interested in this analysis.

Line 90 - Maybe explain better what "looking at the effect of management" means AND what "comparing the performance of the mixed model" means.

Hope this helps! Let me know if you have questions :)

zkamvar commented 8 years ago

Hi @panastheod, just checking in to see if you have any questions about our suggestions.

panastheod commented 8 years ago

Dear Zhian,

Sorry for the late reply. I am waiting for my co-author to sent me the revisions. Most likely in the next days we will sent you and update version.

Panas

On Mon, Jun 6, 2016 at 3:29 AM, Zhian N. Kamvar notifications@github.com wrote:

Hi @panastheod https://github.com/panastheod, just checking in to see if you have any questions about our suggestions.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NESCent/popgenInfo/pull/182#issuecomment-223851328, or mute the thread https://github.com/notifications/unsubscribe/ASQlhs1rpwtiXdU_z4ZEtTOrP5tWdWO1ks5qI3fegaJpZM4IW3b0 .

panastheod commented 8 years ago

Dear Zhian,

I have just uploaded our revised vignette.

All the best,

Panas

On Mon, Jun 6, 2016 at 3:29 AM, Zhian N. Kamvar notifications@github.com wrote:

Hi @panastheod https://github.com/panastheod, just checking in to see if you have any questions about our suggestions.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NESCent/popgenInfo/pull/182#issuecomment-223851328, or mute the thread https://github.com/notifications/unsubscribe/ASQlhs1rpwtiXdU_z4ZEtTOrP5tWdWO1ks5qI3fegaJpZM4IW3b0 .

zkamvar commented 8 years ago

Hi @panastheod, I apologize for the delayed response. I was at a hackathon this past week. I will take a look at the revisions today or tomorrow. @mmlopezu, I have rendered the vignette and am attaching it as a zip file here: LMM-Genetic-Diversity.zip

zkamvar commented 8 years ago

To render this vignette on your own machine, you can copy and paste the following code:

tmp <- tempdir() # create temporary directory
setwd(tmp)       # go there
library('git2r') # load git2r

# Clone the repository and go into the use folder
repo <- clone("https://github.com/panastheod/popgenInfo.git", "popgenInfo") 
setwd("popgenInfo/use")

# Checkout the branch
checkout(repo, "02-05-2016-LMM-Genetic-Diversity")

# Render and show the file
rmarkdown::render("LMM-Genetic-Diversity.Rmd")
file.show("LMM-Genetic-Diversity.html")
zkamvar commented 8 years ago

@panastheod, Thank you for taking the time to revise this submission. Your recent edits have added much clarity to the analysis and I believe you have addressed all of the comments from myself and @mmlopezu. If @mmlopezu approves, we will merge this into repository, clean up any minor issues, and add a link on the main page. Do you have a preference for what the title in the dropdown menu should be?

Thanks, Zhian

panastheod commented 8 years ago

Dear Zhian,

How about "Comparing genetic diversity using mixed models" for a title.

All the best,

Panas

On Mon, Jun 27, 2016 at 8:14 PM, Zhian N. Kamvar notifications@github.com wrote:

@panastheod https://github.com/panastheod, Thank you for taking the time to revise this submission. Your recent edits have added much clarity to the analysis and I believe you have addressed all of the comments from myself and @mmlopezu https://github.com/mmlopezu. If @mmlopezu https://github.com/mmlopezu approves, we will merge this into repository, clean up any minor issues, and add a link on the main page. Do you have a preference for what the title in the dropdown menu should be?

Thanks, Zhian

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NESCent/popgenInfo/pull/182#issuecomment-228828752, or mute the thread https://github.com/notifications/unsubscribe/ASQlhq9kGTSvO6zEWOfGjB6GMxEwaPqKks5qQBLrgaJpZM4IW3b0 .

hlapp commented 8 years ago

I think @mmlopezu is on vacation leave, so in the interest of moving this forward and given thumbs up by @zkamvar I'm going to merge this. I've created a separate issue for adding the link to the menu.

zkamvar commented 8 years ago

@panastheod The vignette is now live at http://popgen.nescent.org/LMM-Genetic-Diversity.html

Thank you for your contribution!

panastheod commented 8 years ago

Great!!

On Wed, Jul 6, 2016 at 10:27 PM, Zhian N. Kamvar notifications@github.com wrote:

@panastheod https://github.com/panastheod The vignette is now live at http://popgen.nescent.org/LMM-Genetic-Diversity.html

Thank you for your contribution!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NESCent/popgenInfo/pull/182#issuecomment-230895879, or mute the thread https://github.com/notifications/unsubscribe/ASQlhgZIWm8fzysQ-k8J-YKFI512KpLZks5qTA-rgaJpZM4IW3b0 .

mmlopezu commented 8 years ago

@panastheod @hlapp @zkamvar

Thanks for working on this while I was away on vacation! I found some typos in the text of the vignette. What is the most efficient way to make those edits/suggestions?

hlapp commented 8 years ago

@mmlopezu can you make a pull request with the corrections?

mmlopezu commented 8 years ago

@hlapp I'll try :)