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

Rename size aesthetic for line-based geoms #3672

Merged
merged 14 commits into from
Jun 15, 2022

Conversation

thomasp85
Copy link
Member

This is a draft PR that seeks to rename the size aesthetic whenever it refers to the width of a stroke. There are two problems with the current setup:

  1. It conflates two distinct aesthetics making it difficult to control each one separately when working with geoms that contain both points and lines/polygons, e.g. boxplot, linerange, and sf.

  2. As the default size scale is area based, scaling of stroke width is not linear by default, and it is needed to use scale_radius() to change that which is super counterintuitive

For the most part we can change this without backward compatibility issues as size can be renamed to linewidth if present in setup_data(). A few geoms have need for both as discussed in 1.) and we can not do any inference there meaning that there might be visual breakage in these cases. The pros outweigh the cons considerably though (IMO).

The aesthetic name linewidth has been chosen as both width and stroke is already in use. linewidth also falls in line with the linetype aesthetic.

Please use this PR for discussions

@thomasp85 thomasp85 added this to the ggplot2 3.4.0 milestone Mar 26, 2021
@thomasp85
Copy link
Member Author

Team - I would like to get this into the next release as it will be primarily focused on cleaning up code, syntax, and internal, and I feel the current behaviour is a major logical issue... Are there any objections to this change in API?

@clauswilke
Copy link
Member

Will this require all extension packages to make changes to their code? I like the idea conceptually, but I think we need to think carefully about how the transition will work. If downstream packages need to change their code, can they easily support the new and the old ggplot2 at the same time?

@thomasp85
Copy link
Member Author

That is a good question to bring up... This will not break any extension packages (I think), but it will also not magically transform them into understanding linewidth as an aesthetic. It is not possible to do an automatic renaming since size is still a valid aesthetic in some cases...

So there will be an API consistency issue between ggplot2 and the extension packages in terms of aesthetic naming until they are updated.

However, I think our dev community has generally reacted quite fast on these types of updates

@thomasp85
Copy link
Member Author

The implementation of this has now been improved significantly from the point-of-view of updating geoms and ensuring a smooth transition.

In order to upgrade a geom implementation to use linewidth instead of size all you need to do is to change size to linewidth in the default_aes field, and set the rename_size field to TRUE. Further, you of course need to update your code wherever it refers to size and change it to linewidth

What you get for free when setting rename_size = TRUE is that ggplot2 will automatically rename any use of the size aesthetic on the fly, inherit size from the global mapping if no linewidth mapping exist, allow scale_size() to be used with the size aesthetic in the geom etc. It will further emit a message to update call to use linewidth. I'm fairly certain this means that there won't be much code breaking and that we can gently coerce users into using the new aesthetic, while also making it very easy for extension developers to update their geoms.

@thomasp85 thomasp85 marked this pull request as ready for review May 24, 2022 12:03
@thomasp85 thomasp85 changed the title WIP: Rename size aesthetic for line-based geoms Rename size aesthetic for line-based geoms May 24, 2022
@thomasp85
Copy link
Member Author

I will go through adding unit tests for all the affected geoms etc, but before that, I'd like some input about the current approach @hadley @clauswilke @yutannihilation

R/layer.r Outdated Show resolved Hide resolved
Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

LGTM!

R/layer.r Outdated Show resolved Hide resolved
Copy link
Member

@yutannihilation yutannihilation left a comment

Choose a reason for hiding this comment

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

Looks great in general, though I'm not yet sure what this change will mean to the extension packages. Minor comments.

DESCRIPTION Outdated Show resolved Hide resolved
R/geom-sf.R Show resolved Hide resolved
R/scale-linewidth.R Show resolved Hide resolved
Copy link
Member

@yutannihilation yutannihilation 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 adding a note. Looks good!

Conflicts:
	tests/testthat/_snaps/draw-key/time-series-and-polygon-key-glyphs.svg
@yutannihilation
Copy link
Member

yutannihilation commented Jun 13, 2022

Just curious, will this eventually change the behavior of size on Geoms that actually have both size and linewidth? I know there's no breaking change here, but #4872 made me think this might need some consideration to avoid ambiguity.

For example, in the case of geom_pointrange(), it's not clear what to do when we want to increase only the size of the points. Does specifying only size achieve this? Or, will this increase both the size of the points and that of the lines? I feel the former behavior is right, but it would be a breaking change.

geom_pointrange(size = 10)

@thomasp85
Copy link
Member Author

At this point the ambiguous geoms will not differentiate but I think we would do that down the line after proper deprecation

@yutannihilation
Copy link
Member

I see. Sounds good to me. Let's file an issue for it when we merge this.

@thomasp85 thomasp85 merged commit 3375667 into tidyverse:main Jun 15, 2022
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.

4 participants