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

Intro_to_Base_R: Switch iris -> penguins dataset #403

Merged
merged 11 commits into from
Mar 4, 2021
Merged

Conversation

cansavvy
Copy link
Contributor

@cansavvy cansavvy commented Mar 2, 2021

Summary:

Closes #317

I went through intro_to_base_R.Rmd and replaced steps that used iris with penguins!

Here's the main drawbacks or changes we may want to discuss:

  1. This does mean we have to import a library sooner -- I think its fine?? See if you agree with how I introduce it.
  2. There's NAs in this dataset so for the mean() step I had to use a na.rm = TRUE I don't like adding in an argument so soon, but if there's a better way to explain it or circumvent it, let me know.

mean(iris$Sepal.Length)
```{r penguins-col-mean, live = TRUE}
# calculate the mean of the bill_length_mm column
mean(penguins$bill_length_mm,
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'm debating about leaving it like this and just gliding right through this argument OR adding some more steps to this and asking the participants whether they notice that their are NAs in this set.

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 guess main question is how much time we want to devote to this NA thing.

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 it is fine as is. This is a place where live coding should work well.

Note that the comment you have here will be stripped out, as only full line comments are kept. In this case, I think this is a good thing! It means that somebody looking at the rendered notebook will see the full explanation, but live participants won't see it before somethin goes 'wrong'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good point. I wasn't thinking about the live-ness part.

@cansavvy cansavvy marked this pull request as ready for review March 2, 2021 14:02
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! A few suggestions on loading the data and a couple wording things, some of which are barely related. Also, art!

Oh, and the package needs to go into renv first.

intro-to-R-tidyverse/01-intro_to_base_R.Rmd Outdated Show resolved Hide resolved
intro-to-R-tidyverse/01-intro_to_base_R.Rmd Outdated Show resolved Hide resolved
intro-to-R-tidyverse/01-intro_to_base_R.Rmd Outdated Show resolved Hide resolved
intro-to-R-tidyverse/01-intro_to_base_R.Rmd Outdated Show resolved Hide resolved
@cansavvy cansavvy mentioned this pull request Mar 2, 2021
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.

A few more little changes, but also one bigger thing ("What do we call things?") that was not part of the intent of this PR, but came up because of it. Feel free to just pull that into a separate issue if you don't want to address it now.

intro-to-R-tidyverse/01-intro_to_base_R.Rmd Outdated Show resolved Hide resolved
We will begin our exploration with the old trusted dataset `iris`, which comes with R.
Learn about this dataset using the standard help approach of `?iris`.
We will begin our exploration with dataset about penguins from the [`palmerpenguins` package](https://allisonhorst.github.io/palmerpenguins/).
To use this dataset, we will need to extract it from the `palmerpenguins` using a `::` (more on this later).
Copy link
Member

Choose a reason for hiding this comment

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

I'm using the word "environment" here but I realized that this notebook uses the word "environment" a couple of different ways, to refer both to the overall interface, and to the variables that are present in the "workspace", shown in the Environment pane.

Suggested change
To use this dataset, we will need to extract it from the `palmerpenguins` using a `::` (more on this later).
To use this dataset, we will load it from the `palmerpenguins` package using a `::` (more on this later) and assign it to a variable named `penguins` in our current environment.

We should try to standardize on a single meaning for each word, if we can. Unfortunately, RStudio uses "workspace" (load/save/clear workspace) and "environment" (the name of the pane) in its interface to mean mostly the same thing, but it does mean that both of those words are problematic to describe the whole shebang, as we do on line 47 (and in the objectives).
Maybe we should call it the "RStudio Interface" which I don't love, but don't have anything better right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This is good thing to streamline, but probably outside the scope of this PR. I'll make an issue for 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.

intro-to-R-tidyverse/01-intro_to_base_R.Rmd Outdated Show resolved Hide resolved
intro-to-R-tidyverse/01-intro_to_base_R.Rmd Outdated Show resolved Hide resolved
intro-to-R-tidyverse/01-intro_to_base_R.Rmd Outdated Show resolved Hide resolved
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! I just suggested adding back na.rm (with a comment).

Co-authored-by: jashapiro <[email protected]>
@cansavvy cansavvy merged commit ca80391 into master Mar 4, 2021
@cansavvy cansavvy deleted the cansavvy/penguins branch March 4, 2021 12:30
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.

Remove iris dataset
2 participants