Export guide_axis() and consider improvements

[hadley]

Currently we have no equivalent to guide_legend() for axes (there is an non-exported guide_axis() but it draws the axes rather than describing their parameters). We should have an exported guide object to make it possible to have new types of axes.

The build in guide_axes() also needs to gain better tools for handling long labels:

• Should haveangle parameter which handles all related theme settings
• Should optionally trim overlapping labels by starting and either end and bisecting inwards (cf check.overlap = TRUE in grid.text()
• Should be able to draw multiple lines of labels, alternating the position to avoid overlaps

And generally look at what vega-lite does for other ideas of useful parameters.

[paleolimbot]

I spent about a day and a half on this, and wanted to leave a few things here for when I pick it back up again. So far I've:

• Refactored guide_axis() to draw_axis(), since that's what the current version actually does (creates a grob)
• Created a guide_axis() implementation that mimics guide_legend() and changed draw_axis() to create a guide, train it, then draw it.
• Rewrote the grob creation code so that it might be easier to add features to (in a way that doesn't cause any dopplegangers to change)

https://github.com/paleolimbot/ggplot2/blob/0267a0aa48863b119f8288f95eac6fe87d89d95f/R/guides-axis.r#L125-L256

I still have to (provided all agree this is a good idea)

• Refactor scale and guide code to treat position guides like other guides so they can be specified within scale_x_*(), scale_y_*(), sec_axis(), and guides()
• Refactor Coord code to use guides the guides specified in scale_x_*(), scale_y_*(), sec_axis(), and guides() instead of using draw_axis()
• Collect input on what (if any) improvements should go in guide_axis(), or if they should go in some other guide function (guide_axis_fancy(), or the like)

For reference, the vega-lite axis label documentation, which includes an option in inward-align labels on the end.

[paleolimbot]

I've gotten this to work two ways now, and I would be grateful for some opinions on how best to go about things moving forward.

Getting guide specifications from the position Scales to Coord$render_axis_(h|v)() is not trivial. Both versions I have working use the Layout to coordinate panel guides, which (like the other guides) are resolved and trained at draw time (i.e., ggplot_gtable()). The Coord is responsible for generating a set of guides that it will receive in Coord$render_axis_(h|v)(), much like it does with panel_params.

I've gotten to versions to work (i.e., pass CMD check with no visual changes), one of which "injects" the panel guides into panel_params$guides in Layout$render() ( master...paleolimbot:issue-3322-guide-axis-panel-params ), and the other of which adds a panel_guides argument to Coord$render_axis_(h|v)() and Facet$draw_panels() ( master...paleolimbot:issue-3322-guide-axis-layout ). The first one I think is way better since it doesn't result in breaking any extensions of Facets or Coords.

It might be possible to avoid any changes outside CoordCartesian (as well as some other guide code) if the guides were generated at build time, but then they wouldn't have access to the same things that the other guides do (layers, default mapping, global guides list).

[paleolimbot]

I suppose I'm curious whether having the Layout coordinate panel guides is appropriate, whether the internal changes in the other guide code that was required to make this work are likely to have effects in other places, and whether or not it would be more prudent to clean up the guide code first (#3329).

[paleolimbot]

I think this is the best place for my current thoughts on design, although if @hadley @thomasp85 and/or @clauswilke can think of a more appropriate place to discuss, let me know! These are my current thoughts in how to get user information about the position guides and get it to where the guides are drawn:

User-facing guide code

• Guides that represent one scale should use the current infrastructure for specifying guides (i.e., scale_(x|y)_*(guide = ...) / guides((x|y) = ...)). The only difference from other guide code is that I think the default should be waiver() in the scale functions so that the coordinate system can pick the default guide.

• Guides that represent more than one scale (e.g., guide_grid()) should be specified in the coord_*() constructor. This is already (sort of) the case for the guide_graticule() in coord_sf(), and I don't see any reason that we need to add any arguments to other coords at this time, since there really isn't any reason to cutomize the guide_grid() that can't already happen in the theme settings. Other panel guides are currently possible as layers (like annotation_logticks() or ggspatial::annotation_scale())...the only difference is that layers do not have access to the theme at draw time.

Internal guide code

• Currently, guides are resolved, trained, exposed to the layers (to draw the glyphs), and turned into grobs at draw time (i.e., in ggplot_gtable()). I don't see any reason that this can't be the case for position guides as well.

• Currently, all guides have exactly one position defined by the theme(). This should remain the case for the current guide code, but for position guides I think that guides should have a vector of positions so that code like guide_graticule(position = c("left", "bottom", "foreground")) and guide_axis(position = c("left", "right")) is possible.

• Currently, all guides are trained with one scale and one scale only. This won't work for all position guides, which I think should be trained using a list of scales (just like that passed to Stat$compute_*()). This implies a different training process for position guides, which I think is necessary anyway. From much experimentation trying to get position scales/guides to survive the existing guide code, I think that new training code should be written, and eventually the rest of the guides should switch to using it. Because position guides are going to need separate training code, I think it makes sense to write them as ggproto() objects whose state is internal (like scales).

Guide <- ggproto(
  "Guide", NULL,
  title = NULL,
  position = NULL,
  aesthetics = NULL,
  data = NULL,
  
  # I *think* this is enough information to perform the merge externally
  # NULL = never merge
  hash = function(self, scale) {
    # title <- scale$make_title(guide$title %|W|% scale$name %|W|% labels[[output]])
    # digest::digest(list(title, self$aesthetics, class(self), scale$get_labels())
    NULL
  },
  
  # encompases the current guide_train() and guide_geom()
  train = function(scales, layers, default_mapping) {
    # self$data <- ...
    # invisible(self$data)
    stop("Not implemented", call. = FALSE)
  },
  
  # position is because guides may look different depending on where they are
  # drawn. It is the caller's responsibility to put the grob in the
  # right place
  draw = function(self, theme, coord, position = NULL) {
    stop("Not implemented", call. = FALSE)
  }
)

• Position guides should be resolved by the coordinate system and added to the panel_params as the guides element. This happens at draw time rather than build time, so it needs to be a new method in the Coord. They should go in the panel_params because they are already everywhere the guides are drawn (i.e., no new arguments!). We are also narrowing in to a point where there is a common structure to panel_params for all coords ($x and $y will soon be scale-like objects always, and $guides will be a list() of trained guides).

CoordCartesian <- ggproto(
  ...,
  setup_panel_guides = function(self, panel_params, guides, params) {
    scales <- panel_params[c("x", "y")]
    panel_guides <- list(
      x = guides$x %||% scales$x$guide %|W|% guide_axis(),
      y = guides$y %||% scales$y$guide %|W% guide_axis(),
      guide_grid(aesthetics = c("x", "y"), position = "foreground")
    )
    # resolve guide objects
    panel_guides <- lapply(panel_guides, validate_guide)
    # set default positions and aesthetics if they aren't set
    panel_guides <- lapply(panel_guides, ...)
    # add to panel_params
    panel_params$guides <- lapply(panel_guides, validate_guide)
    
    panel_params
  }
)

• The Coord should also train the guides, but maybe not in the same place as resolving them. This mostly reflects different information requirements and helps keep all the functions short. Also, this means that coordinate systems that don't participate in the new system are left alone.

CoordCartesian <- ggproto(
  ...,
  train_panel_guides = function(self, panel_params, layers, default_mapping, params) {
    scales <- panel_params[c("x", "y")]
    if (is.null(panel_params$guides)) {
      return()
    }
    
    scales <- panel_params[c("x", "y")]
    lapply(panel_params$guides, function(guide) guide$train(scales, ))
    invisible(panel_params)
  }
)

• The Layout will have to pass the guides, layers, and default_mapping to these methods. These are all available in ggplot_gtable(), which calls the method.

Layout <- ggproto(
  ...,
  setup_panel_guides = function(self, guides, layers, default_mapping) {
    self$panel_params <- lapply(
      self$panel_params,
      self$coord$setup_panel_guides,
      guides, 
      self$coord_params
    )
    
    self$panel_params <- lapply(
      self$panel_params,
      self$coord$train_panel_guides,
      layers, 
      default_mapping,
      self$coord_params
    )
    
    invisible()
  }
)

• Then, the Coord has access to trained guides in its panel_params, which already get passed to Coord$render_*() by the Facet.

CoordCartesian <- ggproto(
  ...,
  render_bg = function(panel_params, theme) {
    guides <- get_guides(panel_params, "background")
    ...
  },
  render_fg = function(panel_params, theme) {
    guides <- get_guides(panel_params, "foreground")
    ...
  },
  render_axis_h = function(panel_params, theme) {
    guides_top <- get_guides(panel_params, "top")
    guides_bottom <- get_guides(panel_params, "bottom")
    list(top = ..., bottom = ...)
  },
  render_axis_v = function(panel_params, theme) {
    guides_left <- get_guides(panel_params, "left")
    guides_right <- get_guides(panel_params, "right")
    list(left = ..., right = ...)
  }
)

Thoughts? Suggestions? Modifications?

[clauswilke]

Does this in any way address the problem that geoms (or stats?) may need access to the trained scales (#3116)? Could they now get that access through the coord, at draw time?

[paleolimbot]

The coord would still only deal with position scales. I've since figured out a way to get access to the non position scales...I forgot to update the issue.

[paleolimbot]

Reflecting on this over the weekend, I now feel that I should avoid ggproto() and guide_grid(), focusing only on x and y guides. They will get trained slightly differently but I think they can use S3 until all the guides are switched over.

[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/