Handle label functions that return character(0)

[thomasp85]

This PR addresses an issue in scales. If a label function converts an NA break to a character(0) label we will get an error later on when we enlist the label list. This is fixed here in the ScaleContinuous$get_labels() function by converting such list elements to empty strings ("") instead.

It can be argued that this should get fixed in scales::label_number_auto() (and perhaps it should as well), but I think it makes sense to have a guard in ggplot2 as well, since otherwise this problem will be carried all the way to guide_axis() and create an unintelligible error there.

[hadley]

What happens if the list contains elements of length > 1?

[thomasp85]

It would fail in the same way, I guess... but I think it is more likely that we have functions that inadvertently convert NA input to character(0).

I can check for > 1 as well and just take the first element to be sure.

[thomasp85]

done... do you think this should also be fixed in scales, or is it behaving as it should?

[hadley]

It's definitely suspicious that scales is returning something of the wrong size, but I don't remember what I was thinking in the scales code. Probably worth merging this expedient fix and filing a scales issue so we don't forget to fix upstream.

[paleolimbot]

Do you have an example offhand of code that fails with the current change? Other than lists of expressions, I don't remember why lists are allowed for labels at all.

It's worth noting that lists of expressions silently fail with the current code on this branch. Labels created with expression() do not, but because one can't put the result of expression() in a tibble or data.frame, programmatically these types of labels are more easily generated as a list() of unevaluated expressions (actually, st_graticule() is the only place I know that does this).

library(ggplot2)

ggplot(mpg, aes(cty, hwy)) +
  geom_point() +
  scale_y_log10(
    limits = c(1, 100),
    breaks = c(1, 10,  100),
    labels = lapply(expression(10^0, 10^1, 10^2), identity)
  )

^{Created on 2020-01-16 by the reprex package (v0.3.0)}

[thomasp85]

It surfaced in scales with

> demo_log10(c(1, 1e7), labels = label_number_auto())
# scale_x_log10(labels = label_number_auto())
#  Error in `$<-.data.frame`(`*tmp*`, ".label", value = c("10", "1 000",  : 
#   replacement has 4 rows, data has 5 

[paleolimbot]

Another place to consider inserting this check is right before the unlist() call:

ggplot2/R/guides-axis.r

Lines 88 to 94 in e214e44

	if (is.list(ticks$.label)) {
	if (any(sapply(ticks$.label, is.language))) {
	ticks$.label <- do.call(expression, ticks$.label)
	} else {
	ticks$.label <- unlist(ticks$.label)
	}
	}

Ideally the unlist() should be in the draw function (so that guide_train.axis() could be used for more complex label specs), but that's a battle for another day. I still think that putting the fix right before the unlist() is better...it makes it clear why every element in the list has to be of length 1 (and fixes the expression problem since that's already checked for).

[thomasp85]

My reason for placing it where I have is to avoid all axis guides (assuming extensions will be made) has to make this correction. Basically I think it is fair that the output of get_labels() returns sensible output instead of relying on downstream functions cleaning it up

[paleolimbot]

That's fair...if so, I think it might be helpful to move the unlist() call to get_labels() (thus defining the output of scale$get_labels() as anything that can be passed to titleGrob()).

(i.e. this plus your checks)

if (is.list(ticks$.label)) { 
   if (any(sapply(ticks$.label, is.language))) { 
     ticks$.label <- do.call(expression, ticks$.label) 
   } else { 
     ticks$.label <- unlist(ticks$.label) 
   } 
 }