Adding names to a margin vector

[dmuenz]

I'd like to suggest that the margin function return a named vector, i.e., with names c("t", "r", "b", "l").

m <- ggplot2::margin(t = 1, r = 2, b = 3, l = 4)
m[3] # returns bottom margin
m["b"] # doesn't work -- returns NA

My motivation is that I'm doing some grob calculations that use, for example, just the bottom margin, and being able to write m["b"] would be more self-documenting than m[3].

To test this out, I modified the margin <- S7::new_class(...) definition by adding the line names(u) <- c("t", "r", "b", "l") right before the S7::new_object(u) call in the constructor function. This works, though there's a performance penalty. On my computer, calling the new margin constructor is about 2 μs slower (median time of 35.6 μs versus 34 μs for the original function). Accessing an individual element of the margin object is about 0.5 μs slower (median time of 2.8 μs versus 2.3 μs), and this 0.5 μs penalty holds whether we're accessing an element by name or by numeric index. My code for these benchmark tests is below the fold. I don't know whether these seemingly small penalties would add up to something significant when building/drawing a full ggplot object.

Thanks for your consideration.

Code and benchmarks

library(ggplot2)

# revised copy of the margin constructor, with named elements
margin_named <- S7::new_class(
  "margin_named",
  parent = S7::new_S3_class(c("simpleUnit", "unit", "unit_v2")),
  constructor = function(t = 0, r = 0, b = 0, l = 0, unit = "pt", ...) {
    ggplot2:::warn_dots_empty()
    lens <- c(length(t), length(r), length(b), length(l))
    if (any(lens != 1)) {
      incorrect <- c("t", "r", "b", "l")[lens != 1]
      s <- if (length(incorrect) > 1) "s" else ""
      cli::cli_warn(c(
        "In {.fn margin}, the argument{s} {.and {.arg {incorrect}}} should \\
        have length 1, not length {.and {lens[lens != 1]}}.",
        i = "Argument{s} get(s) truncated to length 1."
      ))
      t <- t[1]
      r <- r[1]
      b <- b[1]
      l <- l[1]
    }
    u <- unit(c(t, r, b, l), unit)
    names(u) <- c("t", "r", "b", "l") # name the unit vector
    S7::new_object(u)
  }
)

# compare constructing a margin object
bench::mark(orig = margin(t = 1, r = 2, b = 3, l = 4),
            new = margin_named(t = 1, r = 2, b = 3, l = 4),
            check = FALSE)

# make two margin objects, one unnamed and one named
m <- margin(t = 1, r = 2, b = 3, l = 4)
m_named <- margin_named(t = 1, r = 2, b = 3, l = 4)

# with the named margin object, we can access elements by name or number
m_named[3]
m_named["b"]

# compare accessing an element
bench::mark(m[3], m_named[3], m_named["b"], check = FALSE)

# show that naming a margin vector doesn't appear to mess up other parts of the
# ggplot build & draw process
p <- mpg |>
  ggplot(aes(cty, hwy)) +
  geom_point()

p + theme(margins = margin(1, 2, 3, 4, unit = "cm"))
p + theme(margins = margin_named(1, 2, 3, 4, unit = "cm"))

[teunbrand]

Thanks for the suggestion! I don't think it is conventional to have named units in grid, and am therefore hesitant to introduce this convention in ggplot2. The main objection is that while grid units behave vector-like, they aren't true vectors in the way atomic vectors behave or {vctrs} treats them (try creating a 0-length unit 'vector', for example). Because unit construction doesn't preserve names, (try names(unit(c(a = 1, b = 2), "cm"))) I don't think they are supported to a degree that we're comfortable cementing this behaviour in ggplot2.

That said, I do think there is some merit in accessing the different positions by name rather than index. Because we're using an S7 class for the amrgin, we might be able to use computed or dynamic properties to access them.

[dmuenz]

Here's my attempt to implement your idea about using dynamic properties. The revised margin class has dynamic properties t, r, b, and l, each with a getter and setter. In the setter I've tried to preserve the behavior of the overall class constructor, so I truncate an input value to be length 1 and coerce it to be a unit object using whatever unit type (e.g. "cm") it already has.

I also check to see if the user passed the setter a unit object with a different unit type from what was originally set. This would cause an error with a cryptic message, so I check for this situation just to provide a friendlier error message.

library(ggplot2)

# helper function to define dynamic properties for t, r, b, l
new_margin_prop <- function(side, index) {
  S7::new_property(
    class = S7::new_S3_class(c("simpleUnit", "unit", "unit_v2")),
    getter = function(self) self[index],
    setter = function(self, value) {
      # make sure value has length 1
      len <- length(value)
      if (len != 1) {
        cli::cli_warn(c(
          "{.arg {side}} should have length 1, not length {len}.",
          i = "{.arg {side}} gets truncated to length 1."
        ))
        value <- value[1]
      }
      
      # abort if value is a unit object with the wrong unit type
      oldUnitType <- grid::unitType(self[index])
      if (grid::is.unit(value) && grid::unitType(value) != oldUnitType) {
        newUnitType <- grid::unitType(value)
        cli::cli_abort(c(
          "Cannot change unit from {.val {oldUnitType}} to \\
          {.val {newUnitType}} by setting {.arg {side}}.",
          i = 'Consider assignment via numeric index, e.g. \\
          {.code x[{index}] <- unit({as.numeric(value)}, "{newUnitType}")}, \\
          but note that the resulting object will not have class {.cls margin}.'
        ))
      }
      
      # update self
      self[index] <- unit(value, oldUnitType)
      self
    }
  )
}

# revised copy of the margin constructor, with dynamic properties
margin2 <- S7::new_class(
  "margin2",
  parent = S7::new_S3_class(c("simpleUnit", "unit", "unit_v2")),
  properties = list(
    t = new_margin_prop("t", 1),
    r = new_margin_prop("r", 2),
    b = new_margin_prop("b", 3),
    l = new_margin_prop("l", 4)
  ),
  constructor = function(t = 0, r = 0, b = 0, l = 0, unit = "pt", ...) {
    ggplot2:::warn_dots_empty()
    lens <- c(length(t), length(r), length(b), length(l))
    if (any(lens != 1)) {
      incorrect <- c("t", "r", "b", "l")[lens != 1]
      s <- if (length(incorrect) > 1) "s" else ""
      cli::cli_warn(c(
        "In {.fn margin}, the argument{s} {.and {.arg {incorrect}}} should \\
        have length 1, not length {.and {lens[lens != 1]}}.",
        i = "Argument{s} get(s) truncated to length 1."
      ))
      t <- t[1]
      r <- r[1]
      b <- b[1]
      l <- l[1]
    }
    u <- unit(c(t, r, b, l), unit)
    S7::new_object(u)
  }
)

# example usage
m2 <- margin2(t = 1, r = 2, b = 3, l = 4, unit = "cm")

m2
m2@b # access bottom margin
m2[3]

m2@b <- 5 # change the bottom margin, preserving original unit type
m2@b

m2@b <- "3" # this works too
m2@b

m2@b <- c(2, 4) # uses just the 1st element, with a warning
m2@b

m2@b <- "a" # invalid input, results in NA with a warning
m2@b

m2@b <- unit(4, "cm") # this works since we're using the original unit type
m2@b

m2@b <- unit(4, "pt") # this errors since the unit type has changed
m2@b

Some simple benchmark tests suggest that this approach is a little bit slower than my previous approach of just naming the unit vector, for both constructing the margin object and accessing an element by name. However, with the previous approach there was a penalty even when accessing an element by numeric index, while there is no discernable penalty in this case using the new dynamic property approach. These penalties are all on the order of 1 to 5 μs.

Thoughts?

[teunbrand]

Thanks for trying this out, I think this nails the suggestion. There are few nits to pick, but the shape of the solution is right.

I'm assuming the increase in construction time is due to property validation, which wouldn't really make sense for computed/dynamic properties but may need to be invoked anyway. I think benchmarking on the level of a plot would give a more realistic view of the speed penalty. Mostly because constructing new margins has already happened in the default theme, and may not need to be counted. Furthermore, internal code all uses numeric indices for accessing specific margins, so that'd not be affected by the property approach, but might be by the named approach.

Alternatively, if the only goal is code legibility, you can also just declare some helper functions for your own project, without any need for changes in ggplot2.