Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

Remove R script/notebook package installs (Part 1 of N) #698

Merged

Conversation

cansavvy
Copy link
Collaborator

Purpose/implementation Section

What was your approach?

I'm removing R package installations in the scripts in accordance with #690
I'm keeping track of the PRs I file for that issue here and going through the modules alphabetically.

This PR is me having gone through chromosomal-instability - focal-cn-file-preparation, here are the changes in this PR:

  • cnv-chrom-plot: In gistic_plot.Rmd got rid of: install.packages("BiocManager") and BiocManager::install("ggbio") and refreshed gistic_plot.Rmd only.
  • cnv-comparison: is deprecated, so I didn't get rid of the install.packages there
  • focal-cn-file-preparation: Got rid of series of installs in 04-prepare-cn-file.R, re-ran bash run-prepare-cn.sh.

Directions for reviewers. Tell potential reviewers what kind of feedback you are soliciting.

Everything look okay?

Is the analysis in a mature enough form that the resulting figure(s) and/or table(s) are ready for review?

Yes, but the results shouldn't really be different.

Reproducibility Checklist

No changes are needed here.

  • The dependencies required to run the code in this pull request have been added to the project Dockerfile.
  • This analysis has been added to continuous integration.

Copy link
Contributor

@cbethell cbethell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look just about ready to merge 👍

I am just concerned about what appears to be the removal of a plot: focal-cn-file-preparation/plots/consensus_seg_cn_autosomes_stranded_stacked_plot.png
Any idea why this may be the case here?

@cansavvy
Copy link
Collaborator Author

These changes look just about ready to merge 👍

I am just concerned about what appears to be the removal of a plot: focal-cn-file-preparation/plots/consensus_seg_cn_autosomes_stranded_stacked_plot.png
Any idea why this may be the case here?

The script seemed to finish without any errors, but I'm also not super familiar with this code. This may be something I could use your help investigating, @cbethell . What notebook/script creates that plot?

@cbethell
Copy link
Contributor

These changes look just about ready to merge 👍
I am just concerned about what appears to be the removal of a plot: focal-cn-file-preparation/plots/consensus_seg_cn_autosomes_stranded_stacked_plot.png
Any idea why this may be the case here?

The script seemed to finish without any errors, but I'm also not super familiar with this code. This may be something I could use your help investigating, @cbethell . What notebook/script creates that plot?

The script that creates that plot is the rna-expression-validation.R script of the focal-cn-file-preparation module. It's strange to me that it is just that one plot that is missing. I can look into it on your branch to see if I could figure out what's going on there.

@cansavvy
Copy link
Collaborator Author

@cbethell , I'm looking at this line:

--filename_lead "consensus_seg_annotated_cn"_${chromosome_type}_${strategy}

And don't think this file consensus_seg_cn_autosomes_stranded_stacked_plot.png which doesn't have the annotated bit in there, would be saved. Is there somewhere else that you expect this file would be saved from?

As of right now, my best guess is that that file is from an older edition where before you added the annotated part in the name. Let me know if there's somewhere else I should be looking.

@cansavvy
Copy link
Collaborator Author

My other guess is maybe you had previously included the consensus file in this next loop:

filenameLead=("cnvkit_annotated_cn" "controlfreec_annotated_cn")

Let me know if either of my guesses ring a bell, or if not, where in the code you would expect that file should be saved from.

Copy link
Contributor

@cbethell cbethell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbethell , I'm looking at this line:

--filename_lead "consensus_seg_annotated_cn"_${chromosome_type}_${strategy}

And don't think this file consensus_seg_cn_autosomes_stranded_stacked_plot.png which doesn't have the annotated bit in there, would be saved. Is there somewhere else that you expect this file would be saved from?

As of right now, my best guess is that that file is from an older edition where before you added the annotated part in the name. Let me know if there's somewhere else I should be looking.

Ahhh this is it! You are correct. The filenames were changed to include annotated so that plot must have been left over from before that change was made. In that case, this LGTM 🚀

@cansavvy
Copy link
Collaborator Author

cansavvy commented May 19, 2020

I don't think either of my theories regarding consensus_seg_annotated_cn_autosomes_stranded_stacked_plot.png are corrrect. I think it should have been saved. My best guess now is that a step got skipped because of memory intensive requirements. I think my plan is to run this on a bigger boat. What is confusing is why this didn't stop the whole script with the set -e and set -o pipefail specifications. But I could be misunderstanding how these work.

@cansavvy cansavvy closed this May 19, 2020
@cansavvy cansavvy reopened this May 19, 2020
@cansavvy cansavvy requested a review from cbethell May 20, 2020 11:21
@cansavvy
Copy link
Collaborator Author

@cbethell I re-ran the run-prepare-cn.sh script on AWS. It looks like those files were created this time, but can you take another look to make sure everything else with focal-cn-file-preparation looks as you would expect?

Copy link
Contributor

@cbethell cbethell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great now @cansavvy!

There is what looks to be the addition of the consensus_seg_with_ucsc_cytoband_status.tsv.gz but I checked to ensure that none of the data in this file changed and it didn't (just the order of the biospecimen IDs changed and I am guessing this is because the bedtools step does not necessarily retain the order of IDs).

That being said, now it LGTM 🚀

@jaclyn-taroni jaclyn-taroni merged commit 0985505 into AlexsLemonade:master May 21, 2020
@cansavvy cansavvy deleted the cansavvy/remove-r-package-installs branch August 13, 2020 11:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants