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

Update cheatsheet links for intro-R and scRNAseq #521

Merged
merged 23 commits into from
Mar 1, 2022

Conversation

sjspielman
Copy link
Member

@sjspielman sjspielman commented Feb 28, 2022

This PR addresses Issue #515 for the most immediate cheatsheet needs (with other cheatsheet updates coming in future PRs):

  • intro-to-R-tidyverse-cheatsheet.md
  • scRNA-seq-cheatsheet.md

Links have been moved from rdocumentation --> rdrr and otherwise updated when appropriate.

Feedback I know is needed:

  • Agree on updated links?
  • There is one item in the scRNA-seq cheatsheet which I could not update. The cheatsheet references a {scater} function normalize() which does not appear in any {scater} docs, but maybe {scuttle}?
  • It seems these cheatsheets are not rendered at build time, but I'm not clear which pandoc command is used to match the existing format, so there are no PDFs yet.

…or a scater link which I can't find. Future commits will figure out how to knit these into the right PDF format
@jashapiro
Copy link
Member

  • There is one item in the scRNA-seq cheatsheet which I could not update. The cheatsheet references a {scater} function normalize() which does not appear in any {scater} docs, but maybe {scuttle}?

Yes, that is the correct function; it is reexported by scater. I think we can keep it listed as a scater function for now, to reduce cognitive load, but the link can go to the docs in scuttle

@jashapiro
Copy link
Member

I assume that these were just rendered from an rstudio

  • It seems these cheatsheets are not rendered at build time, but I'm not clear which pandoc command is used to match the existing format, so there are no PDFs yet.

Going by the appearance, I would guess these were not rendered by pandoc, but were rendered as html followed by conversion "print to pdf" or equivilent. I would be 100% fine with the rendered versions being replaced by html.

@jashapiro
Copy link
Member

I assume that these were just rendered from an rstudio

  • It seems these cheatsheets are not rendered at build time, but I'm not clear which pandoc command is used to match the existing format, so there are no PDFs yet.

Going by the appearance, I would guess these were not rendered by pandoc, but were rendered as html followed by conversion "print to pdf" or equivilent. I would be 100% fine with the rendered versions being replaced by html.

I take that back: changing to html would require link updates... Not sure we want to do that right now as it will take some time to track those down. Adding html would be fine though!

Copy link
Member

@jashapiro jashapiro left a comment

Choose a reason for hiding this comment

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

Looks good overall. I made some formatting and content suggestions in my comments below, and we will definitely want the PDFs in this PR to preserve current links to those files. As per my previous comment, adding html versions will not hurt.

module-cheatsheets/intro-to-R-tidyverse-cheatsheet.md Outdated Show resolved Hide resolved
| Base `R` | [`c()`](https://www.rdocumentation.org/packages/base/versions/3.6.2/topics/c) | Combine | Combines values into a vector or list. |
| Base `R` | [`library()`](https://rdrr.io/r/base/library.html) | Library | Loads and attaches additional packages to the R environment. |
| Base `R` | [`<-`](https://rdrr.io/r/base/assignOps.html) | Assignment operator | Assigns a name to something in the R environment. |
| Base `R` | [`c()`](https://rdrr.io/r/base/c.html) | Combine | Combines values into a vector or list. |
| Base `R` | [`%in%`](http://www.datasciencemadesimple.com/in-operator-in-r/) | "in" logical operator | Checks if the given value(s) on the left side of the operator are in the vector or other R object defined on the right side of the operator. It returns a logical `TRUE` or `FALSE` statement. |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| Base `R` | [`%in%`](http://www.datasciencemadesimple.com/in-operator-in-r/) | "in" logical operator | Checks if the given value(s) on the left side of the operator are in the vector or other R object defined on the right side of the operator. It returns a logical `TRUE` or `FALSE` statement. |
| Base `R` | [`%in%`](http://www.datasciencemadesimple.com/in-operator-in-r/) | "in" logical operator | Checks if the given value(s) on the left side of the operator are in the vector or other R object defined on the right side of the operator. It returns a logical `TRUE` or `FALSE` statement. |

Do we want to link to https://rdrr.io/r/base/match.html for uniformity? We could link to both...

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually went back and forth on this one. I liked the datasciencemadesimple more because it was just about the operator, but point taken about consistency. I think both is a good call for this one. I can change the link to rdrr, and add into description [This](datsciencemadesimple) is also useful or similar.



### `ggplot2`

Read the `ggplot2` documentation [**here**](https://ggplot2.tidyverse.org/reference/).
Read the `ggplot2` documentation [**here**](https://ggplot2.tidyverse.org/).
Copy link
Member

Choose a reason for hiding this comment

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

Curious about this change. I think the link to the list of all functions may be more useful than the homepage for this use case. This may also apply to the earlier ggplot2 link in the intro cheatsheet (we should be consistent either way).

Copy link
Member Author

Choose a reason for hiding this comment

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

First, this is a general thing to come back to for consistency.

I much prefer presenting the overall landing page for the documentation, sans reference, because it allows them to hop anywhere in the ggplot2 docs without being strictly presented with an overwhelming list of functions, most of which will be unfamiliar to them. Maybe we need a third opinion on this one..

Copy link
Member Author

Choose a reason for hiding this comment

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

Middle ground: we can have it all!

Read the ggplot2 package documentation here, as well as an overall reference for ggplot2 functions here.

Copy link
Member

Choose a reason for hiding this comment

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

Just noting here that you didn't implement the same changes as in the intro here, but I think that is fine. I also noticed that we only apparently add one new ggplot command here, which makes me wonder if we need it at all. Maybe just add geom_vline to the geom_hline section from the intro and remove it from the single cell doc?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just noting here that you didn't implement the same changes as in the intro here, but I think that is fine.

Can't hurt, will do.

Maybe just add geom_vline to the geom_hline section from the intro and remove it from the single cell doc?

Makes sense to me!

Copy link
Member Author

Choose a reason for hiding this comment

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

I've decided to also delete the final description bit that suggests geom_vline() and friends can only be used on top of another geom.

module-cheatsheets/intro-to-R-tidyverse-cheatsheet.md Outdated Show resolved Hide resolved
| `SingleCellExperiment`| [`counts()`](https://bioconductor.org/packages/release/bioc/vignettes/SingleCellExperiment/inst/doc/intro.html#4_Convenient_access_to_named_assays)| Counts| Stores or extracts raw single-cell experiment count data as an assay of the `SingleCellExperiment` object|
| `scran` | [`quickCluster()`](https://rdrr.io/bioc/scran/man/quickCluster.html) | Quick Clustering | Groups similar cells into clusters which are stored in the `SingleCellExperiment` object and are used for the calculation of size factors by `scran::computeSumFactors`|
| `scran` | [`computeSumFactors()`](https://rdrr.io/bioc/scran/man/computeSumFactors.html) | Compute Sum Factors| Returns a numeric vector of computed sum factors for each cell cluster stored in the `SingleCellExperiment` object. The cluster-based size factors are deconvolved into cell-based size factors that are stored in the `SingleCellExperiment` object and used by the `scran::normalize` function for the normalization of each cell's gene expression profile|
| `scater`| [`TODO!!!!! normalize()`](https://www.rdocumentation.org/packages/scater/versions/1.0.4/topics/normalize)| Normalize | Returns the `SingleCellExperiment` object with normalized expression values for each cell, using the size factors stored in the object
Copy link
Member

Choose a reason for hiding this comment

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

When you update this, consider moving it with the other scater functions.

module-cheatsheets/scRNA-seq-cheatsheet.md Outdated Show resolved Hide resolved
module-cheatsheets/scRNA-seq-cheatsheet.md Outdated Show resolved Hide resolved
@jaclyn-taroni
Copy link
Member

Commenting on the PDF, HTML bit only from the context in my email (what could go wrong!) - a concern I have about having both files is that they'll get out of date with each other. So can we add a docs change with how to render via pandoc to a module-cheatsheets/README? A future enhancement could have this rendering as part of the other rendering workflow?

@sjspielman
Copy link
Member Author

sjspielman commented Feb 28, 2022

Going by the appearance, I would guess these were not rendered by pandoc, but were rendered as html followed by conversion "print to pdf" or equivalent.

They are definitely not rendered with pagedown::chrome_print() from what I can tell, because links don't come out blue anymore...and markdown links are wrecked. Attached is an example of a chrome-printed version.
intro-to-R-tidyverse-cheatsheet_chrome_print.pdf

@jaclyn-taroni Yes, this definitely needs to be added to README, once we know where they came from in the first place 🙃

@jaclyn-taroni
Copy link
Member

You're not going to like this answer, but almost certain it's MacDown. It doesn't need to stay that way though.

@sjspielman
Copy link
Member Author

The plot thickens! It's been a bit hard to find concise docs for normalize() and unfortunately I found it here - https://rdrr.io/bioc/scater/man/defunct.html 🤔

@jashapiro
Copy link
Member

The plot thickens! It's been a bit hard to find concise docs for normalize() and unfortunately I found it here - https://rdrr.io/bioc/scater/man/defunct.html 🤔

Looks like we followed that warning and removed it some time ago. So we should change that to logNormCounts, which we do use!

@sjspielman
Copy link
Member Author

sjspielman commented Feb 28, 2022

You're not going to like this answer, but almost certain it's MacDown.

Womp. Confirmed, it was definitely MacDown. And excitingly, it will remain MacDown!

@sjspielman
Copy link
Member Author

Since we're probably going to stick with MacDown PDF export (they look nice!), I'd like to get the final PDFs for these two cheatsheets into this PR too. Once we feel the md's are ready to go, I will commit updated PDFs, and also add a README about this.. "rendering workflow."

@jashapiro
Copy link
Member

Since we're probably going to stick with MacDown PDF export (they look nice!), I'd like to get the final PDFs for these two cheatsheets into this PR too. Once we feel the md's are ready to go, I will commit updated PDFs, and also add a README about this.. "rendering workflow."

Go ahead and commit the PDFs alongside any md changes. No need to wait for multiple rounds of review.

@@ -0,0 +1,8 @@
### Cheatsheets Module
Copy link
Member

Choose a reason for hiding this comment

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

I think this content makes more sense as part of CONTRIBUTING.md than living here.

components/dictionary.txt Outdated Show resolved Hide resolved
@sjspielman
Copy link
Member Author

Looks like a few subheaders had either not previously made in into the TOC, or they had been manually removed.

@sjspielman
Copy link
Member Author

And the dictionary is sad again :/

Copy link
Member

@jashapiro jashapiro 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 good; Thanks for the update to the contributing docs as well!

I had one remaining comment about the lonely ggplot section in the single-cell cheatsheet. I'd be fine with merging it into the intro, but it is up to you. If you do make that update, make sure to rerender both PDFs.

I don't think I need to rereview.



### `ggplot2`

Read the `ggplot2` documentation [**here**](https://ggplot2.tidyverse.org/reference/).
Read the `ggplot2` documentation [**here**](https://ggplot2.tidyverse.org/).
Copy link
Member

Choose a reason for hiding this comment

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

Just noting here that you didn't implement the same changes as in the intro here, but I think that is fine. I also noticed that we only apparently add one new ggplot command here, which makes me wonder if we need it at all. Maybe just add geom_vline to the geom_hline section from the intro and remove it from the single cell doc?

@jashapiro
Copy link
Member

And the dictionary is sad again :/

Surprised it checked the contributing doc, but ah well. My only other comment would have been to change "md" to "`.md`" to avoid making "md" a dictionary word (the spellchecker skips code and I think it knows that backticks denote code.)

CONTRIBUTING.md Outdated
+ Open a md cheatsheet in MacDown, and go to `File` -> `Export` -> `PDF`
+ Save appropriately and voila!

When choosing documentation links to incorporate in cheatsheets, we prefer to use [https://rdrr.io/](https://rdrr.io/) when possible for Base R and Bioconductor, and we prefer to use [https://www.tidyverse.org/](https://www.tidyverse.org/) for `tidyverse` functions.
Copy link
Member

Choose a reason for hiding this comment

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

Get past spellcheck

Suggested change
When choosing documentation links to incorporate in cheatsheets, we prefer to use [https://rdrr.io/](https://rdrr.io/) when possible for Base R and Bioconductor, and we prefer to use [https://www.tidyverse.org/](https://www.tidyverse.org/) for `tidyverse` functions.
When choosing documentation links to incorporate in cheatsheets, we prefer to use [`https://rdrr.io/`](https://rdrr.io/) when possible for Base R and Bioconductor, and we prefer to use [https://www.tidyverse.org/](`https://www.tidyverse.org/`) for `tidyverse` functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh boy those latter tickmarks are wrong anyways, huh. Good catch.

Copy link
Member

Choose a reason for hiding this comment

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

No, my suggestion was wrong with one of them!

components/dictionary.txt Outdated Show resolved Hide resolved
sjspielman and others added 2 commits March 1, 2022 18:04
Co-authored-by: Joshua Shapiro <[email protected]>
CONTRIBUTING.md Outdated Show resolved Hide resolved
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.

3 participants