MEM_outlier

[BrennaF]

Hello,

I'm contributing a vignette (2016-12-13_MEM_outlier.Rmd) with data (MSOD_vignette_data.csv & Hab_Plot.png) for our paper in press with Molecular Ecology Resources. This vignette would fit under the "For SNP data" section, perhaps below the "Signals of Selection" vignette. It could be called "Spatial Outlier Detection". Please let me know if you have questions. I'm new to Git/Github, so may have made a mistake or two while following your contribution guidelines.

Thanks! Brenna

[hlapp]

@BrennaF: thank you very much for the submission! At the time you submitted (and at the point at which you forked the repository to add your changes), we had a build failure due to breaking changes in the pcadapt package (#193). This has now been fixed (#195).

The currently visible build failure in your submission is due to this issue. If you know how to update your fork, rebase your branch, and then update this pull request, please go ahead and do that. If not, we can copy your branch and do that on your behalf.

[zkamvar]

Hi @BrennaF,

Thank you again for your submission. I made some specific line comments above to ensure that the vignette will render properly.

Both the text and the code in the vignette are consistent and read well, giving enough background to understand the problem and demonstrating the usefulness of MSOD and MSR.

I have a couple of suggestions:

• If the data for the habitat selection surface are not too large, it might be good to include them and actively plot them instead of showing an image (though, the text file from Dryad did take up 15MB of memory, which is pretty large by R standards).
• After section 2, it would be nice to show three versions of the Gabriel graph, coloring the nodes with the genotype at L1, L90, and a random locus. I played around with the data and found that the visual is quite striking:

the_colors <- viridis::viridis(3) # You can use any palette here.
op <- par(no.readonly = TRUE)
par(mfrow = c(1, 3), mar = c(1, 1, 1, 1))
plot(nb, coords = Coord)
title("L1")
points(Coord, col = the_colors[Loci[, 1] + 1], pch = 20)
plot(nb, coords = Coord)
title("L2")
points(Coord, col = the_colors[Loci[, 2] + 1], pch = 20)
plot(nb, coords = Coord)
title("L90")
points(Coord, col = the_colors[Loci[, 90] + 1], pch = 20)
par(op)

[zkamvar]

Additionally, for the sake of other reviewers, here's a rendered PDF version: 2016-12-13_MEM_outlier.pdf

[hlapp]

though, the text file from Dryad did take up 15MB of memory, which is pretty large by R standards

Just as a thought, this file would not necessarily have to be deposited in this repo; R does have facilities that allow relatively painlessly to download the file on the fly, as part of rendering the vignette.

[smanel]

I suggest to move the l48 in the section 1 to the introduction paragraph ## Data & packages (l18) since the same idea is developped

L48 (The example data provided here are simplified from a CDPOP (Landguth & Cushman 2010) simulation output file. This particular simulation used an aggregated habitat surface, a selection coefficient of 5%, and individual dispersal of 5% of the surface per generation. These data are from the first replicate of 10 for these simulations. The full simulation data set is available at Dryad doi: 10.5061/dryad.v0c77, with more information on the simulation scenarios in Forester et al. 2016.)

[hlapp]

Somehow there is now a pr/194 branch in the repo, and it looks to me like that is what the CircleCI test runs from. Since @BrennaF updating this PR didn't also update that branch, CircleCI wasn't seeing any changes. I have now pushed her updates to that branch, which seems to done the trick.

For the record, I've also run the build myself locally, which resulted in the following error:

Quitting from lines 26-32 (2016-12-13_MEM_outlier.Rmd) 
Error in setwd(dir) : cannot change working directory
Calls: <Anonymous> ... process_group.block -> call_block -> block_exec -> in_dir -> setwd
In addition: Warning messages:
1: In grep("\n", x) : input string 195 is invalid in this locale
2: In grep("\n", x) : input string 204 is invalid in this locale
3: In grep("^(---|\\.\\.\\.)\\s*$", lines) :
  input string 195 is invalid in this locale
4: In grep("^(---|\\.\\.\\.)\\s*$", lines) :
  input string 204 is invalid in this locale
5: In grepl(chunk.begin, lines) :
  input string 195 is invalid in this locale
6: In grepl(chunk.begin, lines) :
  input string 204 is invalid in this locale
7: In grepl(chunk.end, lines) : input string 195 is invalid in this locale
8: In grepl(chunk.end, lines) : input string 204 is invalid in this locale

Execution halted

This is after I pulled the latest version of the hlapp/rpopgen container. It seems @zkamvar was able to build the vignette without error, so I'm not sure what might have gone wrong in me running this. @zkamvar did you simply run make test (or make html, which should be synonymous) or did you run something else to build the vignette?

[hlapp]

It looks like the build on CircleCI did replicate the error. I'll have to go look through the code to see what might be wrong; one candidate is the setwd() call, this should normally not be needed, and be avoided. @zkamvar any hints?

[hlapp]

Alright, I can get past the directory and file errors by following the changes @zkamvar marked. Then this error stops the build:

Quitting from lines 53-64 (2016-12-13_MEM_outlier.Rmd) 
Error in eval(expr, envir, enclos) : 
  could not find function "scores.listw"
Calls: <Anonymous> ... handle -> withCallingHandlers -> withVisible -> eval -> eval

[zkamvar]

@hlapp,

did you simply run make test (or make html, which should be synonymous) or did you run something else to build the vignette?

I ran it locally using the protocol outlined in the wiki 😳

Error in eval(expr, envir, enclos) : could not find function "scores.listw" Calls: ... handle -> withCallingHandlers -> withVisible -> eval -> eval

I believe this is a function in the adespatial package, which we don't have in the container, if you add adespatial, it should work.

[hlapp]

I believe this is a function in the adespatial package, which we don't have in the container, if you add adespatial, it should work.

Indeed, that worked (with the changes @zkamvar put in comments).

[BrennaF]

Hello all -- I tried to upload a new version of the vignette, but I'm not sure it worked? Let me know if I need to do anything on my end.

@zkamvar I believe I successfully implemented the code to pull the habitat file from the dryad repository (and modified it so I could plot it as an ascii file). The only thing is that I can't figure out how to remove all of the white space around the plots in the Rmarkdown html render. Just cosmetic...but annoying.

Thanks!

[zkamvar]

@BrennaF, Thanks for making the changes! Here are some answers to your questions:

I tried to upload a new version of the vignette, but I'm not sure it worked? Let me know if I need to do anything on my end.

It looks like you made the commit to your master branch. Don't worry too much about that. Just go here https://github.com/BrennaF/popgenInfo/blob/MEM_outlier/use/2016-12-13_MEM_outlier.Rmd, click edit and copy and paste the raw text from the master branch. I can also do that for you if you wish.

The only thing is that I can't figure out how to remove all of the white space around the plots in the Rmarkdown html render. Just cosmetic...but annoying.

You can use the chunk option out.width like so:

```{r habitat_plot, out.width = "100%"}

Also, in your recent upload, on line 78, you should make the following change:

- Habitat <- raster(paste0(zipd,"\\L10H5R1_aa.asc"))     # The Habitat ascii file
+ Habitat <- raster(paste0(zipd,"/L10H5R1_aa.asc"))      # The Habitat ascii file

Of course, don't worry too much about getting this done immediately, we need to update the docker image to include both the raster and viridis packages. Happy Holidays! :snowman:

[BrennaF]

Thanks @zkamvar! I went ahead and made a few changes, and uploaded the revised version to my MEM_outlier branch this time (I think). Let me know if you see a problem.

I spent some time troubleshooting that figure -- chunk options didn't help with the whitespace between main title and plot (although setting width to 100% is an overall improvement). Adding a pin=c(4,4) argument to par helped reduce whitespace in Rstudio, but kept throwing an error when I used knit. So I'm just going to leave it for now since it is only cosmetic.

[zkamvar]

Thanks @zkamvar! I went ahead and made a few changes, and uploaded the revised version to my MEM_outlier branch this time (I think). Let me know if you see a problem.

Excellent! I see no problems so far. The build failed, but I expected that because the docker container wasn't up to date. I updated it and made a small change to your vignette, so that it gets rebuilt from the updated container.

I spent some time troubleshooting that figure -- chunk options didn't help with the whitespace between main title and plot (although setting width to 100% is an overall improvement). Adding a pin=c(4,4) argument to par helped reduce whitespace in Rstudio, but kept throwing an error when I used knit. So I'm just going to leave it for now since it is only cosmetic.

Ah! I didn't realize you were looking to adjust the space between the plot and title. For those matters, using this cheat sheet on base graphics is super helpful.

[BrennaF]

Fantastic @zkamvar - and thank you for the cheat sheet - super helpful! I'm not going to bother with it any more for now, though. The error seemed to be specific to image.plot related to the raster package ("Error in graphics::par(plt = bigplot)").

Thanks for all your help!

[zkamvar]

Everything looks good from here! The checks have passed and both @smanel and I have given our reviews, so I will make a couple more changes to add this to the dropdown menu, add reviewer names, and then merge this into the website. Thanks again for the submission @BrennaF. Happy Boxing Day!

[zkamvar]

Oh, I also figured out how to fix the whitespace around the figures. The default width and height in Rmarkdown documents is 7x7 inches. By setting fig.height = 4 in the chunk options, it removes the whitespace.

[BrennaF]

Wonderful! Thanks for figuring out that (easy!) fix for the whitespace. Do I need to fix it or have you already done it?

Also - I believe @smanel wasn't finished with her review yet so we will want to wait to post it until we get any final edits from her (beginning of January).

And happy Boxing Day to you too @zkamvar! Thanks again for all your help with this!

[zkamvar]

I can easily fix it, so don't worry too much about that.

That's a good point about @smanel's pending review! I will wait to merge until everything is finalized.

On Dec 26, 2016, at 13:45 , Brenna Forester notifications@github.com wrote:

Wonderful! Thanks for figuring out that (easy!) fix for the whitespace. Do I need to fix it or have you already done it?

Also - I believe @smanel https://github.com/smanel wasn't finished with her review yet so we will want to wait to post it until we get any final edits from her (beginning of January).

And happy Boxing Day to you too @zkamvar https://github.com/zkamvar! Thanks again for all your help with this!

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

[smanel]

L28: I also needed to download the package: yaml

[zkamvar]

Comments from @smanel:

L7: I think that it would be nice to have a short explanation on the power Spectrum. I am aware that it is explained later. But if possible to introduce it shortly here, it would be wonderful.

L52: I would prefer that the initial ref for MEM are given one time in the vignette and not only Wagner et al. 2016

L110: Not necessary to define again the power Spectrum. Already defined just above.

L112: Unclear: z-scores of which tests? Add outliers after detection

L230: population and deme are the same? If yes better to keep population.

[BrennaF]

I just incorporated all of the edits from @smanel - thanks for the feedback Stéphanie!

@zkamvar, I'm not sure I updated the code correctly - I see a pending check? Let me know if I need to do anything!

[BrennaF]

And now the check says "passed" :)

[zkamvar]

Fantastic! Once @smanel gives her approval, we'll go ahead and get everything incorporated!

[smanel]

L54. Indicate in bracket either the name of the R object or the name of the function use (as nbdist). It can help to follow what is done

[smanel]

l7: when you define the power spectrum : can you indicate why this mesure is useful? I feel that it is the heart of the method and a short explanation would be very useful to understand the method: the power Spectrum is used as an indicator of xxx l91: a MEM l156: Unclear of both variables?

After these minor corrections, I approve the vignette.

[BrennaF]

Addressed comments from @smanel on L54, L7, L91, L156. I also added reviewer names to the contributor list. Thank you @zkamvar, @smanel, and @hlapp for your help!

[hlapp]

Sounds like we're ready to merge, @zkamvar? I'm wondering whether this should be a standard merge or whether the commits are best squashed into one and merged. Is it important to maintain the exact history of the changes in response to review comments? I lean to no, but can go either way.

[zkamvar]

I think squashing makes sense; I don't think we really need the exact history of the changes in response, we will always have this pull request.

Before I do that, I'm going to delete the PNG figure that's in the data directory because it's not needed anymore.

[zkamvar]

Congratulations @BrennaF, your vignette is now live at http://popgen.nescent.org/2016-12-13_MEM_outlier.html 🎆 🎉

[BrennaF]

So exciting!! Thank you so much for all of your help @zkamvar, @hlapp, and @smanel !!