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

ggplot minimal example #2

Merged
merged 9 commits into from
Jan 22, 2018
Merged

Conversation

FelixTheStudent
Copy link
Contributor

Hi Jon,

thanks for our little chat on twitter. It was in fact very trivial to use shades in ggplot, but perhaps seeing it can motivate beginners to install and play with your package.

I leave it up to you whether you include this or not, in any case it was fun to play around with it!

Cheers,

Felix

Added minimal ggplot2 example using mtcars dataset.
added subhead for clarity
added minimal ggplot example and dichromatic heading for clarity
minimal ggplot2 example now loads required packages to be stand-alone
minimal ggplot2 example now loads required packages to be stand-alone
Copy link
Owner

@jonclayden jonclayden left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! It looks good, but I think a few tweaks are needed

README.Rmd Outdated
@@ -53,6 +53,7 @@ c("red","blue") %>% brightness(0.6) %>% saturation(seq(0,1,0.25)) %>% swatch

This operation takes the original two colours, reduces their brightness to 60%, assigns a whole series of saturation levels to the result, and then passes it to `swatch` for visualisation. Notice that the pipeline is combinative (like the base function `outer`), returning each combination of parameters in a multidimensional array. The final shades are arranged in two rows by `swatch`, for convenience.

### dichromat function and colour-blindness
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather this was removed for now, as it doesn't apply to everything below it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine by me, I'll take it out!

README.Rmd Outdated
@@ -87,6 +88,19 @@ Finally, you can calculate perceptual distances to a reference colour, as in
distance(c("red","green","blue"), "red")
```

### Minimal ggplot2 example
```r
Copy link
Owner

Choose a reason for hiding this comment

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

I think knitr requires braces around the r. We probably need to also increase the figure height a bit for these cases—otherwise they look a bit squashed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Being the gitHub newby I am, I wasn't aware the Rmd gets knitted - is that resulting in the readme's html display people see on your repo? If so, what is the md for, why do we maintain both?

README.Rmd Outdated
ggplot(mtcars, aes(mpg, qsec, col=cyl)) + geom_point() +
scale_color_manual(values=gradient("viridis",3) %>% dichromat)
ggplot(mtcars, aes(cyl, mpg, fill=cyl)) + geom_boxplot() +
scale_fill_manual(values=gradient("viridis",3) %>% dichromat)
Copy link
Owner

Choose a reason for hiding this comment

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

These shouldn't go through dichromat()—that is only for simulating colour blindness, but the scales carry more information for those with normal colour perception in their original form

README.Rmd Outdated
library(magrittr)
library(ggplot2)
library(shades)
data("mtcars")
Copy link
Owner

Choose a reason for hiding this comment

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

magrittr isn't needed here (see below), and I don't think the dataset needs to be loaded explicitly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I included data("mtcars") to make it very obvious and clear for beginners. But now I agree with you, even ggplot's absolute beginner tutorials at tidyverse.org do not explicitly do it. I'll take it out! This is fun. Thanks for doing this with me, I'm inappropriately excited given how small this contribution is - it's just fun to learn how to contribute on gitHub!

Copy link
Contributor Author

@FelixTheStudent FelixTheStudent left a comment

Choose a reason for hiding this comment

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

Thanks for doing this with me! Not sure why we have both .md and .Rmd, google didn't spit out immediately useful explanations - can you shortly explain?

README.Rmd Outdated
@@ -87,6 +88,19 @@ Finally, you can calculate perceptual distances to a reference colour, as in
distance(c("red","green","blue"), "red")
```

### Minimal ggplot2 example
```r
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Being the gitHub newby I am, I wasn't aware the Rmd gets knitted - is that resulting in the readme's html display people see on your repo? If so, what is the md for, why do we maintain both?

README.Rmd Outdated
library(magrittr)
library(ggplot2)
library(shades)
data("mtcars")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I included data("mtcars") to make it very obvious and clear for beginners. But now I agree with you, even ggplot's absolute beginner tutorials at tidyverse.org do not explicitly do it. I'll take it out! This is fun. Thanks for doing this with me, I'm inappropriately excited given how small this contribution is - it's just fun to learn how to contribute on gitHub!

@jonclayden
Copy link
Owner

I'm happy to explain. In brief, GitHub converts the contents of README.md (which needs to have that name) to HTML and shows the result on a repository's home page. But if it contains executable R code then it is desirable to execute it each time the README is updated, to make sure that everything still works. So README.Rmd is the canonical source, which is run through knitr to produce README.md. But both need to be under version control so that the code can be re-run easily, and so that GitHub presents the knitted version. Hadley gives more detailed explanation in his R packages book.

@jonclayden
Copy link
Owner

The changes I've suggested are fairly trivial, and I could easily make them myself, but I thought you might find it useful to see what a PR code review looks like, for future reference. The workflow now is that you make any changes you want locally, you push them to the master branch in your repository (FelixTheStudent/shades), and this PR will then be automatically updated. I can then approve the changes and merge the PR into my version of the repository.

@jonclayden
Copy link
Owner

Hi, @FelixTheStudent! Just checking whether you want to make these changes, or you'd rather I merged the PR as-is, and then made them myself?

@FelixTheStudent
Copy link
Contributor Author

Hi @jonclayden, thanks for your explanations and patients, sorry it took so long. You are absolutely right in saying it's helpful to go through PR code review! I'll get on it right away.
Also found Hadley's chapter very useful, thanks for pointing me there. I would then edit my Rmd and knit it, and commit the result to my Repo, correct?

@jonclayden
Copy link
Owner

Exactly - many thanks. Please don't overwrite the existing figures that haven't changed, though (as in d69fcf4).

@FelixTheStudent
Copy link
Contributor Author

Please don't overwrite the existing figures that haven't changed, though

Good you would mention it. Knitting the Rmd file automatically overwrites them and git recognizes them as changed (not sure why, though - due to different creation date perhaps?).
I will now check out the previous versions from your d69fcf4 commit to remedy this; unless you have a better suggestion? Almost there now :)

@jonclayden
Copy link
Owner

That's fine. They will differ for trivial reasons, such as the exact software versions on your system, but you can choose not to commit them. Having already done so, yes, checking out the versions from commit ec573e6 and making a new commit should work.

@jonclayden jonclayden merged commit e9f56f4 into jonclayden:master Jan 22, 2018
@jonclayden
Copy link
Owner

Excellent - the PR is now merged! Thanks again for your efforts.

@FelixTheStudent
Copy link
Contributor Author

Amazing! Thanks so much for guiding me through that first PR experience. That was fun!

@FelixTheStudent
Copy link
Contributor Author

Good luck with your future projects, see you some time 🗡️

@FelixTheStudent
Copy link
Contributor Author

🗡️ should be this: :D

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.

None yet

2 participants