WIP: Pass actual data to annotation_raster() and annotation_custom() instead of dummy data

[yutannihilation]

Fix #3120

This PR does fix these two annotations to have xmin, xmax, ymin and ymax as data so that scale transformations takes effect (this is the same strategy as annotate()).

• annotation_raster()
• annotation_custom()

On the other hand, this PR doesn't modify annotation_map() and annotation_logstick() because:

• annotation_map(): we don't know what to do with the reversed scales for map yet (c.f. scale transformations won't work for coord_sf() or coord_map()). Besides, annotation itself should not be transformed, but I don't know how to implement this for maps.
• annotation_logstick(): the document says "These tick marks probably make sense only for base 10"

library(ggplot2)
library(patchwork)

data <- data.frame(x = c(20, 85, 42, 78, 33, 74),
                   y = c(43, 40, 52, 56, 44, 71))

p <- ggplot(data) +
  geom_point(aes(x = x, y = y)) +
  annotate(geom = "rect", xmin = 8, xmax = 12, ymin = 38, ymax = 42) +
  annotation_custom(
    grob = grid::circleGrob(
      r  = grid::unit(1, "npc"),
      gp = grid::gpar(col  = "black",
                      fill = "white",
                      lwd = 2)),
    xmin = 48, xmax = 52, ymin = 48, ymax = 52
  )

p * (p + scale_x_reverse())

rainbow <- matrix(hcl(seq(0, 360, length.out = 50 * 50), 80, 70), nrow = 50)
p <- ggplot(mtcars, aes(mpg, wt)) +
  geom_point() +
  annotation_raster(rainbow, 15, 20, 3, 4)

p * (p + scale_x_reverse())

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

[clauswilke]

Yes, this is what I mean. Maybe add a regression test that checks for correct behavior under scale_x_reverse(). Can this be done without adding a visual test, just by checking the layer_data()?

[yutannihilation]

Thanks, I'll need to modify the expectations here...

ggplot2/tests/testthat/test-annotate.r

Lines 33 to 51 in 18be30e

	test_that("annotation_* has dummy data assigned and don't inherit aes", {
	custom <- annotation_custom(zeroGrob())
	logtick <- annotation_logticks()
	library(maps)
	usamap <- map_data("state")
	map <- annotation_map(usamap)
	rainbow <- matrix(hcl(seq(0, 360, length.out = 50 * 50), 80, 70), nrow = 50)
	raster <- annotation_raster(rainbow, 15, 20, 3, 4)
	dummy <- dummy_data()
	expect_equal(custom$data, dummy)
	expect_equal(logtick$data, dummy)
	expect_equal(map$data, dummy)
	expect_equal(raster$data, dummy)
	expect_false(custom$inherit.aes)
	expect_false(logtick$inherit.aes)
	expect_false(map$inherit.aes)
	expect_false(raster$inherit.aes)
	})

[yutannihilation]

I feel

• annotation_custom(), anotation_raster(), and annotation_map() should be modified to have data.
• annotation_logstick() can be kept as is.

[thomasp85]

Can you add a test for making sure that annotations doesn't affect the scales of the plot?

[yutannihilation]

Thanks, let me think how to test it...

[yutannihilation]

@thomasp85 Sorry, I didn't get your point. What do you mean by "annotations doesn't affect the scales of the plot"? I think annotations does affect.

library(ggplot2)
library(patchwork)

p <- ggplot(mtcars, aes(x = wt, y = mpg)) + geom_point()

p1 <- p + annotate("text", x = 4, y = 25, label = "Some text")
p2 <- p + annotate("text", x = 100, y = 100, label = "Some text")

p1 * p2

^{Created on 2019-02-19 by the reprex package (v0.2.1.9000)}

[yutannihilation]

Opps, I got it. That's the very reason we needed different annotations than annotate(). Sorry, I didn't understand this.

ggplot2/R/annotation-custom.r

Lines 6 to 10 in 7bafc45

	#' This is a special geom intended for use as static annotations
	#' that are the same in every panel. These annotations will not
	#' affect scales (i.e. the x and y axes will not grow to cover the range
	#' of the grob, and the grob will not be modified by any ggplot settings
	#' or mappings).

So, by their definitions ("the grob will not be modified by any ggplot settings or mappings"), is this PR invalid? It sounds user's responsibility to provide the correct coordinates.

[thomasp85]

I think the intent of the PR is good. annotations should respond to scale transformations, but they should not affect the scales in any way (at least that's my feeling, @hadley can you chime in?)

The only way they can respond to transformations is to have the positional data in the actual layer data, so I'm beginning to think that the best way would be to exempt annotation layers from the scale training somehow

[yutannihilation]

Sure. One possible choice is to transform data in draw_layer() because there are scales in layout and data with xmin, xmax, ymin, and ymax, mapped by Layer$compute_geom_2().

yutannihilation@092f39f

But, it seems I need to wait for #3116 to get some conclusion?

[hadley]

I don't remember; but if the docs say annotations don't affect the scale limits, then we need to stick with that. My vague recollection is that this makes it possible to add annotations to free scales: e.g. imagining labelling cities on spatial data where each panel has different limits.

[thomasp85]

I agree that we should keep the behaviour in line with the documentation, but do you think we should let it respond to scale transformations? (I do)

[hadley]

Oh yes, definitely. It should respond but not affect.

[yutannihilation]

Thanks, then I'll keep trying this.

I think we have two possible directions:

1. Make xmin, xmax, ymin and ymax to aesthetics and add some option to Layer (or somewhere) to choose not to use the layer data for training scales. (This is @thomasp85 suggests).
2. Keep them as parameters and transform them using the trained scale in draw_layer().

The latter one is related to the discussion in #3116. Though this is possible with the status quo as this only needs x and y scales, I feel I should wait for the discussion so I can implement this cleaner.

[clauswilke]

I agree. The second approach seems cleaner. It would also fit in with another thought I've always had about annotations: You really want to be able to access either the data coordinate system or the plot coordinate system, depending on the context. Sometimes you want to place an annotation at a specific data location. Other times, you want to place it in a specific location on the canvas, e.g. 10% away from the top right corner. The second approach would allow for both, if you transform or not depending on some switch that you add to the annotation geoms.

[yutannihilation]

You really want to be able to access either the data coordinate system or the plot coordinate system, depending on the context.

Oh, I haven't come up with this, but this is quite true. Thanks.

[clauswilke]

Ideally you'd also want to be able to use units in the second scenario.

[baptiste]

Maybe worth considering is making annotation_custom a special case of a geom_custom, like most (all?) other annotations. There's an (ugly) attempt in egg, which I guess also serves to illustrate the need for mixed native+relative units (here there is no way to specify the width of the grob in data space),

library(ggplot2)
library(egg)

data <- data.frame(x = c(20, 85, 42, 78, 33, 74),
                   y = c(43, 40, 52, 56, 44, 71))

dummy <- data.frame(x=50, y = 50)
dummy$grob <- list(grid::circleGrob(
  r  = grid::unit(2, "mm"),
  gp = grid::gpar(col  = "black",
                  fill = "white",
                  lwd = 2)))

p <- ggplot(data) +
  geom_point(aes(x = x, y = y)) +
  annotate(geom = "rect", xmin = 8, xmax = 12, ymin = 38, ymax = 42) +
  geom_custom(data = dummy, aes(x, y, data=grob), grob_fun = identity)

p + scale_x_reverse()

[thomasp85]

@yutannihilation are you awaiting #3175 for finishing this off? if so, I'll not add it to the 3.2.0 milestone

[yutannihilation]

Yes, I think I need to wait for it. Sorry I wasn't clear about the current status here.

[thomasp85]

No worries... will push this to after the next release

[scu111]

Any updates, please? The fix doesn't seem to work for me.

[teunbrand]

I suspect this might be fixable by using the position scales nested in panel_params without the need for #3175 to be implemented. Should we continue to wait for this?

[teunbrand]

Closing this PR in favour of #6182.