Closed jthomasmock closed 1 year ago
Super, thanks! Do you want to acknowledge your contribution in the changelog by adding (jthomasmock, #78)
?
Super, thanks! Do you want to acknowledge your contribution in the changelog by adding
(jthomasmock, #78)
?
Done!
There's one more issue, and it's a bit of a tricky one (maybe). I invented alternative aesthetics so people could customize the points and vertical lines separately. See for example here: https://github.com/wilkelab/ggridges/blob/add4f72193a4a0142d0ee90724f5427c037d7911/R/geoms.R#L110-L116
In the code, I'm using either the point/line specific one or the general one, whichever is provided: https://github.com/wilkelab/ggridges/blob/add4f72193a4a0142d0ee90724f5427c037d7911/R/geoms.R#L167
It seems wrong to me to only change size
to linewidth
and not change vline_size
also to vline_linewidth
. However, with the latter change, it may break some people's code, as there's no fallback. So what do we do? Maybe just breaking the code is better than carrying fallback code forward for all eternity.
I saw that, but in the moment decided to leave alone since it may be much more disruptive (and I didn't know the code base that well).
I guess that the size -> linewidth change is just as disruptive, since the size option was removed...through that lens I'd be in favor of "this is the breaking change patch" rather than keeping linesize around for longer.
AFAIK if we did rename linesize, if a user used the old argument it would not error but rather throw a warning that the aesthetic is ignored. That feels ok in that we're not causing errors but a warning and clarifying in changelog that the option is renamed.
Yes, no code will break. Just some figures will no longer look as expected. And this applies only to the extent that anybody even uses this feature. I don't know if that's the case. It's rather obscure, so may not be a big deal to just break it.
Yes, no code will break. Just some figures will no longer look as expected. And this applies only to the extent that anybody even uses this feature. I don't know if that's the case. It's rather obscure, so may not be a big deal to just break it.
I'm of the opinion that changing linewidth with aes() is a less-used feature - I can try to add to the PR a specific warning if line_size
arg is used? That would be a more explicit warning but would also require you to remove that warning at some point in future.
Note that whether we use an aesthetic inside of aes()
or rather as a parameter directly in the geom doesn't make any difference to how it's handled downstream at drawing time.
The more I think about it, the more I believe we should just rename vline_size
to vline_linewidth
throughout. The warning would be great if it's not too much work, as it would tell people they need to change their code rather than just silently producing a changed plot. I don't think the warning has to ever be removed if it says something like "vline_size
has been deprecated; please use vline_linewidth
instead."
Ok, I'll try and update this PR.
Plan:
Convert all instance of vline_size
to vline_linewidth
and take a pass at detecting the vline_size
argument for a special warning. I am confident in the first half, and less so in the second but will take a shot.
I think I can use logic similar to this:
I think this is the right logic for the check: https://github.com/tidyverse/ggplot2/blob/63125db184b6e7d5516007574b5d15acbfd8f470/R/geom-.r#L241-L247
Unfortunately, since I'm replacing and not actually deprecating the arguments are simply not valid, and the warnings are intercepted by missing args warnings. I could add the arguments back alongside the new vline_width
, but that also further complicates the code.
With the logic as outlined above in the ggplot2
examples:
# aesthetic ignored
Warning message:
In geom_density_ridges(aes(vline_size = cyl), quantile_lines = TRUE) :
Ignoring unknown aesthetics: vline_size
# parameter ignored
Warning message:
In geom_density_ridges(quantile_lines = TRUE, vline_size = 1) :
Ignoring unknown parameters: `vline_size`
While that's not our "intended" message - it does at least tell the user that their parameter/aesthetic is no longer valid - leading them to check the arguments/aesthetics. It's the same warning for vline_size
and size
now, where linewidth
or vline_width
work silently as intended.
library(ggplot2)
library(ggridges) #<- local dev version
base_plt <- mtcars |>
ggplot(aes(x = mpg, y = factor(cyl)))
# not a valid aesthetic
base_plt +
geom_density_ridges(aes(vline_size = cyl), quantile_lines = TRUE)
#> Warning in geom_density_ridges(aes(vline_size = cyl), quantile_lines = TRUE):
#> Ignoring unknown aesthetics: vline_size
#> Picking joint bandwidth of 1.38
# not a valid parameter
base_plt +
geom_density_ridges(quantile_lines = TRUE, vline_size = 1)
#> Warning in geom_density_ridges(quantile_lines = TRUE, vline_size = 1): Ignoring
#> unknown parameters: `vline_size`
#> Picking joint bandwidth of 1.38
# swap and works
base_plt +
geom_density_ridges(aes(vline_width = cyl), quantile_lines = TRUE)
#> Picking joint bandwidth of 1.38
# swap and works
base_plt +
geom_density_ridges(quantile_lines = TRUE, vline_width = 3)
#> Picking joint bandwidth of 1.38
Created on 2022-11-21 by the reprex package (v2.0.1)
I think you can keep the old variant (vline_size
) alive by listing it as optional_aes
here:
https://github.com/wilkelab/ggridges/blob/add4f72193a4a0142d0ee90724f5427c037d7911/R/geoms.R#L121
Then you can implement the check as in ggplot.
If that doesn't work, I'd probably give vline_size
a default value (in default_aes
), even if it's not used anymore. I think that's better than throwing the seemingly random error that vline_size
is an unknown parameter.
I have kept the vline_size
arg as NULL and provided a deprecation warning.
library(ggplot2)
library(ggridges) #<- local dev version
base_plt <- mtcars |>
ggplot(aes(x = mpg, y = factor(cyl)))
# not a valid aesthetic
base_plt +
geom_density_ridges(aes(vline_size = cyl), quantile_lines = TRUE)
#> Picking joint bandwidth of 1.38
#> Warning: Use of the `vline_size` or `size` aesthetics are deprecated, please use
#> `linewidth` instead of `size` and `vline_width` instead of `vline_size`.
Created on 2022-11-22 by the reprex package (v2.0.1)
Thanks for all your effort! We're almost there, but there's one small issue remaining. The aesthetic vline_width
only applies to vertical lines, whereas linewidth
applies to all lines. In your last reprex, when setting vline_size
you can see how it changes all lines, not just the vertical lines, whereas in earlier reprexes you made that wasn't the case. The issue is that in your check function you overwrite linewidth
with vline_size
. I think we need two separate checks, one for size
(which gets copied to linewidth
) and one for vline_size
(which gets copied to vline_width
).
Super, thanks! If the CI runs without issues I'll merge.
Great: r-lib/actions/setup-r@v1 is deprecated. Please update your workflow to use the 'v2' version.
Let me know if you're Ok to take that on also. It should probably be a separate PR.
Ok, only the windows check fails, with the error message I posted, so no reason to assume anything is wrong with this PR. I'll go ahead and merge. Thanks again!
ggridges 0.5.5
ggplot2::expand_scale()
withggplot2::expansion()
in vignettes to avoid deprecation warnings in ggplot2 0.3.3size
argument in line functions withlinewidth
to avoid deprecation warnings in ggplot2 0.3.4..density..
withggplot2::after_stat(density)
to avoid deprecation warnings in ggplot2 0.3.4Closes #78