Skip to content

Multiply colourbar linewidth parameters with *.pt (#4314) #4318

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

Merged
merged 10 commits into from
May 25, 2022

Conversation

teunbrand
Copy link
Collaborator

This PR aims to set straight a small inconsistency in the way the colourbar guide handles linewidths and fixes #4314.
It multiplies the frame.linewidth and ticks.linewidth parameters with .pt before giving these to gpar().
Defaults are inversely transformed to not cause visual changes for users and in tests.

@teunbrand teunbrand changed the title Multiple colourbar linewidth parameters with *.pt (#4314) Multiply colourbar linewidth parameters with *.pt (#4314) Jan 16, 2021
@clauswilke
Copy link
Member

All the tests are failing. You probably need to regenerate the documentation.

Also, could you mention this in NEWS.md, including that it will cause visual changes if people are using something other than the default? (And maybe explain in 1 sentence how to fix this, by adding / .pt.)

@teunbrand
Copy link
Collaborator Author

Yes thank you that seems to help some tests to be successful. There seems to be an issue with geom_sf() and the North Carolina boundaries that appear in some versions/platforms.

@thomasp85 thomasp85 added this to the ggplot2 3.4.0 milestone Mar 25, 2021
@hadley hadley added the visual change 👩‍🎨 Rendering change that will affect look of output label Mar 15, 2022
@hadley
Copy link
Member

hadley commented Mar 17, 2022

Do you happen to have access to the github code search beta (https://cs.github.com)? If so, it'd be worth doing a little searching to guess how frequently these arguments are used. This is technically a visual change, but I suspect so few people use it we could squeeze into a patch release.

@teunbrand
Copy link
Collaborator Author

Do you happen to have access to the github code search beta

Personally I do not. However, googling ggplot2 "frame.linewidth" site:github.com yields 10 hits, 3 of which are outside ggplot2's repo. These are the following and I only expect (1) to have a visual change:

  1. https://github.com/amitjavilaventura/plotmics/blob/8414a25017479c961b235cd9e2b4fe602d6ac229/R/expressionHeatmap.R#L169
  2. https://github.com/tylermorganwall/rayshader/blob/master/R/plot_gg.R (but the value appears to be 0, which shouldn't change)
  3. ggplot2 imports when used in a package eliocamp/ggnewscale#21

Doing the same for ggplot2 "ticks.linewidth" site:github.com yields 18 results, the following of which are outside of ggplot2's repo:

  1. Same as (3) for frame.linewidth: ggplot2 imports when used in a package eliocamp/ggnewscale#21
  2. https://github.com/mgimond/Coronavirus_sample_processing
  3. https://github.com/sbashevkin/WQ-discrete/blob/master/Shiny%20app/app.R
  4. https://github.com/LLNL/2022_DVRFS_microbiome/blob/main/06.BetaDiversity.r
  5. https://github.com/clauswilke/dataviz/blob/master/visualizing_associations.Rmd

I expect 2-5 to be affected in some way.

All in all, I think that there isn't a lot of code on github that use these arguments, and only 1 lives in a package (the very first link in this post).

@thomasp85
Copy link
Member

@hadley do you have any objections to merging this in for next release? It seems the downstream repercussions are pretty minimal...

@hadley
Copy link
Member

hadley commented May 11, 2022

@thomasp85 we probably could. I'm just a little worried about the potential for causing unexplained visual changes that will be hard for folks to track down.

@thomasp85
Copy link
Member

yeah... I do think this is a very minor risk, though... If we fold this in together with the size->linewidth change we can make a general remark about how lines may have changed width for various reasons with the update

@hadley
Copy link
Member

hadley commented May 12, 2022

@thomasp85 sure, sounds good to me.

@thomasp85 thomasp85 merged commit 2a13934 into tidyverse:main May 25, 2022
@thomasp85
Copy link
Member

Thanks @teunbrand !

@teunbrand teunbrand deleted the colourbar_linewidths branch May 25, 2022 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
visual change 👩‍🎨 Rendering change that will affect look of output
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Colourbar tick and frame linewidths
4 participants