Feature request: type stability in internals, specifically at `unlist()` operations

[teunbrand]

I was experimenting with having S3 classes based on the vctrs package as xy coordinates and extending the scales to support these alternative coordinates (link to specific use case ). However, I ran into the problem that unlist operations in ggplot code drop relevant attributes and recursively flatten the data. This would be preventable if unlist operations on S3 classes preserve type stability.

One of the pieces of code that I'm struggling with, is updating/training limits, which needs to go through the scale_apply() function, wherein data is unlisted.

I've asked a question on stackoverflow to try and get around this from the vctrs end that illustrates my struggle with unlist and vctrs classes. I'm also aware that the vctrs package has its own syntax for unlist operations.

I think that type stability in general would be a good thing, and would allow for a richer set of ggplot extensions that support vctrs-based S3 classes. Therefore, I'd like to propose more type stable equivalents of unlist() in the ggplot internals.

[yutannihilation]

Could you provide a reprex (or at least a pseudo-reprex)? I feel it might make some sense to use vctrs, but, since I have zero knowledge about GRanges, I can't imagine why it's bothering you...

[teunbrand]

Sure thing! I've adapted the code below from the question on stack overflow, but added in another field. This is the behaviour I encounter when using a vctrs_rcrd derivative with unlist(), and the same happens in for example the scale_apply() function in ggplot:

library(vctrs)

# Setting up a constructor
facvec <- function(f) {
  vec_assert(f, partial_factor())
  new_rcrd(fields = list(f1 = f, f2 = f), class = "facvec")
}

# Setting up how facvec's should be combined
# Based on: https://vctrs.r-lib.org/articles/s3-vector.html
vec_ptype2.facvec <- function(x, y, ...) UseMethod("vec_ptype2.facvec", y)
vec_ptype2.facvec.default <- function(x, y, ..., x_arg = "x", y_arg = "y") {
  vec_default_ptype2(x, y, x_arg = x_arg, y_arg = y_arg)
}
vec_ptype2.facvec.facvec <- function(x, y, ...) {
  facvec(f = vec_c(field(x, "f1"), field(y, "f1")))
}

# Just for prettier printing
format.facvec <- function(x) {
  paste0(field(x, "f1"), ":", field(x, "f2"))
}

# Some dummy data
a <- facvec(factor(c("a", "b")))
b <- facvec(factor(c("c")))

# The problematic operation
unlist(list(a, b))
#> f11 f12 f21 f22  f1  f2 
#>   a   b   a   b   c   c 
#> Levels: a b c

# What I would like the unlist to return
c(a, b)
#> <facvec[3]>
#> [1] a:a b:b c:c

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

As you can see the facvec class is dropped upon the unlist() operation since the vctr_rcrd is truly a list with vector-like response to most functions (like the POSIXlt class). Also, the unlist() operation returns 6 elements, whereas ideally there would be 3.

Just to be clear, I'm not asking for support for the GRanges class since that is pretty specific for bioconductor (and doesn't play nice at all with base::data.frame) and would probably be better suited for ggplot extensions.

However, I think it would be neat if S3 classes based on the vctrs package could be passed along with little to no modification in the internals of ggplot. I don't know how feasible this would be though, but I would be happy to contribute where possible. I can imagine it would be difficult to replace every unlist by some type-stable equivalent, given the complexity of the code and tests.

[yutannihilation]

Thanks! I think your claim is valid to some extent; we are not 100% happy with lapply() + unlist() and probably waiting for some vctrs-based solution (e.g. map_vec()?). However, I'm not immediately sure whether Scale can be extended to support such class that cannot be simply coerced to numeric or character...

@thomasp85 @clauswilke
What do you think?

[clauswilke]

I think in principle scales could produce output other than numeric or character. For example, I've worked with image scales that convert data values into image objects. I think what would be necessary to move forward is to try to implement such a scale and make the required changes to the ggplot2 code base to see how much of a change is required. When I implemented the image scale, I noticed that the legend code couldn't handle it, and I modified it accordingly. Everything else worked just fine out of the box.

[yutannihilation]

Thanks, the image scale sounds interesting. Then, I'll keep thinking what would be the proper fix for this issue...

[clauswilke]

Code is here:
https://github.com/clauswilke/ggtextures/blob/master/R/scale-image.R

With usage examples here:
https://github.com/clauswilke/ggtextures
(Look at the examples that explicitly call scale_image_manual().)

[teunbrand]

I've experimented a little with getting vctrs-classes past scale training and the following seems to work for me and not yield any unit testing errors; with exception of the sf-based tests, since I haven't managed to install that package. Changed lines marked with ### at the end.

scale_apply <- function(data, vars, method, scale_id, scales) {
  if (length(vars) == 0) return()
  if (nrow(data) == 0) return()

  if (any(is.na(scale_id))) {
    abort("`scale_id` must not be `NA`")
  }

  scale_index <- unname(split(
    seq_along(scale_id),
    factor(scale_id, levels = seq_along(scales))
  ))

  lapply(vars, function(var) {
    pieces <- lapply(seq_along(scales), function(i) {
      scales[[i]][[method]](data[[var]][scale_index[[i]]])
    })
    # Join pieces back together, if necessary
    if (!is.null(pieces)) {
      o <- order(unlist(scale_index))[seq_len(sum(lengths(pieces)))] ###
      do.call("c", pieces)[o] ###
    }
  })
}

Two things to note here:

1. Using do.call("c", pieces) invokes the vec_c method for vctrs-classes, I think, so this is a possible alternative without adding the vctrs-package as dependency. I can't think of a reason why any of the side effects of unlist would be needed, so replacing it with the do.call("c", ...) seems relatively safe. With side effects, I mean the recursion and dropping attributes.
2. Subsetting the result of the order operation, i.e. [seq_len(sum(lengths(pieces)))] was necessary because unlike base R, where subsetting with out-of-bounds indices would yield NA, the vctrs-classes don't allow out-of-bounds indices for subsetting (by default). I don't know why these NAs would be necessary in the current code, but I don't think dropping the NAs is a big problem.

Now for my case, I still have to fix the interaction of the coordinates with the scale, but that probably won't require modifications to the ggplot code itself.

[clauswilke]

@teunbrand I suggest you prepare a pull request with these changes, so we can formally review it. However, be aware that we're late in the release cycle for the next release and this change will have to wait until after (late January, most likely).

[teunbrand]

@clauswilke Thanks, I will! The only thing I would like to still figure out before I prepare a PR, is to confirm that this change is sufficient to achieve my goals. I'll report back here as soon as I know.

[teunbrand]

After some trial and error, I can confirm that this is the only change needed to non-exported objects to -in theory- allow vctrs-classes to remain type stable.

To actually get it to work, I had to implement ggproto objects inheriting from ScaleContinuous and CoordCartesian to get around some other problems. These are exported so that won't be a problem.

Optionally, it would be neat if the Range ggproto object could be exported such that custom training can occur, but it is only a little bit of code to copy so it is no real barrier.

[clauswilke]

I believe Range is supposed to switch over to the version in scales. Not sure what the timeline of that is.