-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Update economics data #2962
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 economics data #2962
Conversation
* Define col_types for read_csv * Pin time series to fix end date * Use usethis::use_data Also ungroups economics-long. This was already present in the code, but not in the saved data set.
Currently, https://github.com/search?q=org%3Acran+ggplot2+economics+extension%3AR&type=Code Yet, I'm not sure when we can consider it's OK to update the existing data... Might it be a choice to rename the data so that the users can notice the change when they see the error |
I think the changes are sufficiently small that this will not cause problems. |
Here's another need to update or replace the existing library(ggplot2)
ggplot(economics_long, aes(date, value01, colour = variable)) +
geom_line()
#> Warning in grouped_indices_grouped_df_impl(.data): Detecting old grouped_df
#> format, replacing `vars` attribute by `groups` Created on 2018-12-28 by the reprex package (v0.2.1) This seems not so urgent. Fortunately, |
@clauswilke As you commented on #3146, what do you think about this pull request? Do you think we can have a chance to update |
I'm less concerned here. I think the main issue is the grouping, and in the unlikely case that somebody currently depends on the preexisting grouping they can easily add it. With the |
Oh, and I think the warning about old groupings is awkward and should be fixed one way or another by the next release. |
Hmm, I mainly worried about the subtle changes of values, which might supprise somebody that the results of some calculation (mean, p-value, or whatever) don't match some examples in books or websites. But, in terms of visual appearances, it seems OK. library(ggplot2)
tmp <- tempfile(fileext = ".rda")
download.file("https://raw.githubusercontent.com/tidyverse/ggplot2/master/data/economics_long.rda", destfile = tmp)
economics_long_old <- local({load(tmp); economics_long})
ggplot(economics_long_old, aes(date, value01, colour = variable)) +
geom_line()
#> Warning: Detecting old grouped_df format, replacing `vars` attribute by
#> `groups` download.file("https://raw.githubusercontent.com/tidyverse/ggplot2/0639906ebe36dec7694ca9e905f9fa7f903671a9/data/economics_long.rda", destfile = tmp)
economics_long_new <- local({load(tmp); economics_long})
ggplot(economics_long_new, aes(date, value01, colour = variable)) +
geom_line() Created on 2019-02-17 by the reprex package (v0.2.1) |
Maybe I worried too much. Considering we need to fix the grouping anyway, I now feel this needs to be merged. Thanks Claus for the comment. |
It looks like the |
Yeah, I was wondering about that part... (probably what's changed was |
Sorry, you are right. # Download from http://research.stlouisfed.org
library(readr)
library(dplyr, warn.conflicts = FALSE)
library(purrr)
library(tidyr)
library(dplyr)
library(ggplot2)
series <- c("PCE", "POP", "PSAVERT", "UEMPMED", "UNEMPLOY")
url <- paste0("http://research.stlouisfed.org/fred2/series/", series, "/downloaddata/", series, ".csv")
fields <- map(url, read_csv,
col_types = cols(
DATE = col_date(format = ""),
VALUE = col_double()
)
)
economics_new <- fields %>%
map2(tolower(series), function(x, series) setNames(x, c("date", series))) %>%
reduce(inner_join, by = "date") %>%
filter(date <= as.Date("2015-04-01"))
data("economics", package = "ggplot2")
for (s in tolower(series)) {
p <- ggplot(mapping = aes(date, !!sym(s))) +
geom_line(data = economics, colour = "red") +
geom_line(data = economics_new, colour = alpha("black", 0.6)) +
ggtitle(s)
plot(p)
} Created on 2019-02-17 by the reprex package (v0.2.1) |
Actually,
|
Yes, so I think the right strategy is to update now but save the original data file into the repo so there won't be any future data updates anymore. |
Agreed. |
@hadley Could you update this when you have time, following this Claus's suggestion? Or, if you are busy, I can take this over.
|
@yutannihilation it would be wonderful if you could take this over for me. You should be able to push into this PR if you get the dev version of usethis then do |
@hadley I didn't know these functions, thanks! I'll do it. |
It seems the data got another revise after this PR had been created. |
@hadley
but it seems the changes are in all columns. Could you confirm the changes (see #2962 (comment)) are still acceptable? |
Yeah, it's fine. |
Thanks! Then I'm merging this. |
This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/ |
Also ungroups economics-long. This was already present in the code, but not in the saved data set.