Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sofia's Edits #9

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Sofia's Edits #9

wants to merge 1 commit into from

Conversation

SI18
Copy link

@SI18 SI18 commented Nov 29, 2021

Install additional packages - install.packages("knitr"), install.packages("devtools"), BiocManager::install("limma"), BiocManager::install("GEOquery"), BiocManager::install("ComplexHeatmap"),
The rest of the code runs.
Based on this code, it is hard to understand what is the result and what the original question was. I also do not clearly understand the logic behind arranging the code in this order.

Install additional packages -  install.packages("knitr"), install.packages("devtools"), BiocManager::install("limma"), BiocManager::install("GEOquery"), BiocManager::install("ComplexHeatmap"),
The rest of the code runs. 
Based on this code, it is hard to understand what is the result and what the original question was. I also do not clearly understand the logic behind arranging the code in this order.
@SI18 SI18 marked this pull request as draft November 29, 2021 00:35
@SI18 SI18 marked this pull request as ready for review November 29, 2021 00:36
Copy link
Collaborator

@PAMatusiak PAMatusiak left a comment

Choose a reason for hiding this comment

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

Sofia, I see that you found a vital error in Tim's code. I found the same error, and to fix it, I used the same code you did. Go team!

My only recommendation is that you insert your BiocManager:: lines of code (lines 22-23) at the beginning, in the first chunk, before load.all (line 19). I get why you put it where you did, but if load.all wont work until you use BiocManager, you might as well put BiocManager first, right?

@Goralsth
Copy link
Collaborator

I agree with Paige on this. Everything else looks good

@richard-cassidy
Copy link
Collaborator

Good catch on the package installations! I agree that the original code could have been annotated a bit more clearly (especially in the last chunk). Good work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants