-
-
Notifications
You must be signed in to change notification settings - Fork 269
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
Change deprecated ggplot2 syntax to current recommendations #1026
Conversation
rstan/rstan/R/stan_plot.R
Outdated
base <- | ||
ggplot2::ggplot( | ||
plot_data$samp, | ||
ggplot2::aes_string(x = "iteration", y = "value", color = "chain") | ||
ggplot2::aes(x = .data$iteration, y = .data$value, color = .data$chain) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's no need for .data$
with aes()
. This applies everywhere.
ggplot2::aes(x = .data$iteration, y = .data$value, color = .data$chain) | |
ggplot2::aes(x = iteration, y = value, color = chain) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the .data$
is necessary to avoid global variable name NOTEs in CRAN checks
@bwiernik Can you address the comment above and rebase this on the |
rstan/rstan/R/stan_plot_helpers.R
Outdated
graph <- base + ggplot2::geom_violin(color = .NUTS_CLR, fill = .NUTS_FILL) | ||
if (chain == 0) return(graph) | ||
chain_clr <- color_vector_chain(ncol(df_x))[chain] | ||
chain_fill <- chain_clr | ||
chain_data <- data.frame(x = as.factor(df_x[, chain]), y = df_y[, chain]) | ||
graph + ggplot2::geom_violin( | ||
data = chain_data, | ||
mapping = ggplot2::aes_string("x", "y"), | ||
mapping = ggplot2::ggplot2::aes(x = .data$x, y = .data$y), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mapping = ggplot2::ggplot2::aes(x = .data$x, y = .data$y), | |
mapping = ggplot2::aes(x = .data$x, y = .data$y), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am guessing this is OK, although I batched a few suggestions.
I removed another From
See vignette("ggplot2-in-packages"). |
Thanks @bwiernik! |
Is this good to merge now? |
Were the linewidth changes merged already at some point? I don’t see them in the Files Changed view, but they are in the commit history. The Files Changed view now just shows aes and .data stuff, which looks good to go. |
It looks like all of the commits but the most recent are showing as already merged. |
Yes, I tested this in the |
Summary:
ggplot2 recently added a
linewidth
argument to replacesize
for line geoms and elements. Currently, loading rstan or a package that loads it produces a deprecation warning about thesize
argument. This PR replacessize
withlinewidth
for line geoms and elements in the rstan plotting functions. The PR also updates a few other instances of deprecated ggplot2 syntax with the recommended alternatives (replacingaes_string()
withaes(.data$...)
, replacing"..density.."
withafter_stat(density)
).Intended Effect:
No visual changes to plot output. Removal of deprecation warnings on package load.
How to Verify:
Output is unchanged for the following plots with the current
develop
version and this PR:Documentation:
https://github.com/tidyverse/ggplot2/blob/main/NEWS.md
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):
Brenton M. Wiernik
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: