Make position guides customizable

[paleolimbot]

This is a PR to address #3322, and allows the same syntax that is currently used to customize non-position guides to be used for position guides. This allows the strategies for dealing with overlapping labels that were implemented in #3375 to actually be used (closes #3281). Currently, these only work for CoordCartesian, but could easily be adapted to CoordTrans as well.

The main changes are:

• Added two methods to Coords: setup_panel_guides() and train_panel_guides(). These are both called from Layout$setup_panel_guides() at draw time. Both Coord methods return modified versions of the panel_params, where the guides are kept.
• Added guide_axis() and guide_none() constructors, which return list S3s similar to the other guides.
• Added S3 methods for guide_axis() and guide_none() objects that exist for all other guides.
• Added an S3 generic guide_transform(), which allows position guides access to the coord and panel_params.

Some problems include:

• A circular dependency (the guide object is kept in the panel params, but needs access to it so that it can call Coord$transform()). I don't know how to get around this, but am open to suggestions.
• Theoretically one could specify two guides at a single position (e.g., two position guides at the left of the panel). I can't get the gtable code to draw this (see this reprex ). Currently the code draws the first guide and issues a warning (there is a test for this).

A reprex giving the main syntax:

library(ggplot2)

# plot with overlapping text
p <- ggplot(mpg, aes(cty * 100, hwy * 100)) +
  geom_point() +
  facet_wrap(vars(class))

# axis guides can be customized in the scale_* functions or
# using guides()
p + scale_x_continuous(guide = guide_axis(n_dodge = 2))

p + guides(x = guide_axis(n_dodge = 2))

# can also be used to add a duplicate guide
p + guides(x = guide_axis(n_dodge = 2), y.sec = guide_axis())

^{Created on 2019-07-05 by the reprex package (v0.2.1)}

[hadley]

I think this looks good. I'd say take another pass at clarifying anything that's confusing now that you've had a break from it, and lets get a review from @thomasp85.

[paleolimbot]

I re-took a look through and I don't see anything that doesn't make sense to me. Initially I was worried that organizing the guides a named list where the names are aesthetics would be slow, but I did some profiling and that doesn't seem to matter. After rereading the code, I think panel_params$guides should stay a list() with names as aesthetics because it is more consistent with the structure of panel_params. If this doesn't make sense anybody else though, it could be changed such that the names of the list are positions.

From the brief benchmarking I did, I found that this PR slows down building because it adds two calls per panel to Scale$get_breaks() (I think this is because it's calling labelling::extended() each time, which is performing a while-loop-based search for optimal breaks). I think that rendering is also slower by a few ms per panel because of S3 dispatch to guide_gengrob(). With x and y breaks specified manually, this PR doesn't cause any build slowdown (see details). I think that tackling the Scale$get_breaks() bottleneck is important but is maybe more appropriate for another PR, and that the render slowdown might be fixed if the guides are ggproto instead of S3.

------------------------------------------------------------------

library(ggplot2)

# plot with overlapping text
p <- ggplot(mpg, aes(cty * 100, hwy * 100)) +
  geom_point() +
  facet_wrap(vars(class))

bench <- ggdebug::ggbench(quos(p), min_iterations = 50)
bench[bench$stage %in% c("build", "draw"), ]

########################

# branch master
# A tibble: 2 x 14
  stage expression     min  median `itr/sec` mem_alloc `gc/sec` n_itr  n_gc total_time result memory
  <fct> <bch:expr> <bch:t> <bch:t>     <dbl> <bch:byt>    <dbl> <int> <dbl>   <bch:tm> <list> <list>
1 build f           40.1ms  43.8ms     22.1      2.8MB    23.0     50    52      2.26s <ggpl… <df[,…
2 draw  f          163.3ms 195.4ms      5.00    4.07MB     7.70    50    77        10s <gtab… <df[,…
# … with 2 more variables: time <list>, gc <list>

########################

# branch issue-3322-pos-guide-cust
# A tibble: 2 x 14
  stage expression     min  median `itr/sec` mem_alloc `gc/sec` n_itr  n_gc total_time result memory
  <fct> <bch:expr> <bch:t> <bch:t>     <dbl> <bch:byt>    <dbl> <int> <dbl>   <bch:tm> <list> <list>
1 build f           48.7ms  55.8ms     16.6     3.07MB    21.2     50    64      3.02s <ggpl… <df[,…
2 draw  f          173.2ms 204.3ms      4.75    4.65MB     8.75    50    92     10.52s <gtab… <df[,…
# … with 2 more variables: time <list>, gc <list>


------------------------------------------------------------------

p <- ggplot(mpg, aes(cty * 100, hwy * 100)) +
  geom_point()

# branch master
# A tibble: 2 x 14
  stage expression    min median `itr/sec` mem_alloc `gc/sec` n_itr  n_gc total_time result memory time 
  <fct> <bch:expr> <bch:> <bch:>     <dbl> <bch:byt>    <dbl> <int> <dbl>   <bch:tm> <list> <list> <lis>
1 build f          10.9ms 12.2ms      79.8    2.34MB     22.5    39    11      489ms <ggpl… <df[,… <bch…
2 draw  f          19.6ms 21.3ms      47.0     2.9MB     36.9    28    22      595ms <gtab… <df[,… <bch…
# … with 1 more variable: gc <list>

############################

# branch issue-3322-pos-guide-cust
# A tibble: 2 x 14
  stage expression    min median `itr/sec` mem_alloc `gc/sec` n_itr  n_gc total_time result memory time 
  <fct> <bch:expr> <bch:> <bch:>     <dbl> <bch:byt>    <dbl> <int> <dbl>   <bch:tm> <list> <list> <lis>
1 build f          13.4ms 15.6ms      57.6    2.36MB     22.4    36    14      625ms <ggpl… <df[,… <bch…
2 draw  f          22.2ms 22.9ms      42.9    3.35MB     50.4    23    27      536ms <gtab… <df[,… <bch…
# … with 1 more variable: gc <list>


------------------------------------------------------------------

# breaks set
p <- ggplot(mpg, aes(cty * 100, hwy * 100)) +
  geom_point() +
  # same breaks but set instead of calculated
  scale_x_continuous(breaks = seq(1000, 3500, 500)) +
  scale_y_continuous(breaks = c(2000, 3000, 4000))
  
################################
  
# branch master
# A tibble: 2 x 14
  stage expression    min median `itr/sec` mem_alloc `gc/sec` n_itr  n_gc total_time result memory time 
  <fct> <bch:expr> <bch:> <bch:>     <dbl> <bch:byt>    <dbl> <int> <dbl>   <bch:tm> <list> <list> <lis>
1 build p          10.4ms 12.1ms      82.7    1.95MB     20.7    40    10      484ms <ggpl… <df[,… <bch…
2 draw  p          20.6ms 21.6ms      45.3     2.9MB     35.6    28    22      618ms <gtab… <df[,… <bch…
# … with 1 more variable: gc <list>

############################

# branch issue-3322-pos-guide-cust
# A tibble: 2 x 14
  stage expression    min median `itr/sec` mem_alloc `gc/sec` n_itr  n_gc total_time result memory time 
  <fct> <bch:expr> <bch:> <bch:>     <dbl> <bch:byt>    <dbl> <int> <dbl>   <bch:tm> <list> <list> <lis>
1 build p          10.9ms 12.5ms      69.6    1.97MB     22.0    38    12      546ms <ggpl… <df[,… <bch…
2 draw  p            21ms 22.7ms      44.3    3.36MB     56.4    22    28      497ms <gtab… <df[,… <bch…
# … with 1 more variable: gc <list>

[paleolimbot]

Some more benchmarking suggests that I'm wrong about S3 dispatch and labelling::extended() being slow. For sure, ViewScale() construction and drawing the guides is slower than it was before.

[thomasp85]

Shouldn't all these helpers below be placed in coord-.r they seem global to all coords?

[paleolimbot]

Right now ViewScales and guide_axis() are only used by CoordCartesian. I think the code could get used in all the Coords eventually, but they aren't used anywhere else yet. I could still easily move them to coord-.r if that's the best place for them.

[paleolimbot]

Thanks @thomasp85 for the super thorough review of this! To your questions:

• I believe any package that subclasses CoordCartesian will break between this PR and the one implementing the ViewScale. I think that position scale subclasses will also break because their guide property will be NULL (this could be fixed to treat NULL like waiver() or use it instead of waiver()).

• Automatically rotating labels is not implemented...it requires writing code that checks for text overlap, which is outside of my skillset at the moment. When guides are extensible, I think it would be better suited to an extension package with Rcpp (I'm pretty sure some code to detect the overlaps is in ggrepel, written in C++).

I think I've addressed your comments, but let me know if you'd like a different approach or if I forgot a comment!

[thomasp85]

There will be facilities for fast text-box dimension calculation down the line without going into C++ so we should be able to get this into ggplot2 proper at some point (not needed for this PR though)

Do you have an overview of which packages this will affect? If we could reach out to them as soon as possible we can make sure they are ready for the next release

[thomasp85]

I think, with the one minor remaining change request, that we should merge this in and begin bracing for impact 🙂 It is better to get a lot of hands on this before we make a release...

any objections @hadley ?

[paleolimbot]

The only subclass of a position scale I know about is ggforce::ScaleContinuousPositionUnit...I don't know of any CoordCartesian subclasses in extension packages, and I can't think of a way to find out other than installing a few thousand packages and grepping the namespaces. I can work on that sometime in the next few weeks.

I think more so than subclasses, extension packages that do things with the built and rendered ggplot2 objects are likely to break (I'm thinking of plotly here, which broke as a result of a previous PR I did).

[thomasp85]

When you've added a NEWS item you are free to merge... then we'll begin working through any possible consequences

[smouksassi]

@paleolimbot after this merge some code that used to work is failing removing the faceting works but why now facet with free_x is not ? the breaks and labels have the same length

library(ggplot2)
ggplot(mtcars,aes(mpg ,disp)) +
  geom_point()+
facet_grid(~cyl,scales="free_x")+
  scale_x_continuous(breaks=c(10,15,20,21,25,30),
                     labels=c("10","15","20","21","25","30") )
#> Error: Breaks and labels are different lengths
       
ggplot(mtcars,aes(mpg ,disp)) +
  geom_point()+
    scale_x_continuous(breaks=c(10,15,20,21,25,30),
                     labels=c("10","15","20","21","25","30") )

^{Created on 2019-10-07 by the reprex package (v0.3.0)}

[clauswilke]

@smouksassi Could you file a separate issue please? I can confirm this problem.

[thomasp85]

FWIW, we are aware of it as it has cropped up in revdepchecks. @paleolimbot is working through these right now

[smouksassi]

@clauswilke let me know if we still need to open a new issue it seems it is already taken care of?

[clauswilke]

I think it never hurts to have an explicit issue of a known problem. @paleolimbot can close it when he submits his fix.

[lock]

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/