Add add_ggplot method

[schloerke]

Followup to #3815

I did not have the separation at the correct point and I did not explain it well. Trying again.

---

Context

Let's say foo is a class defined by a package other than ggplot2. foo objects also inherit from gg. bar objects doc not inherit from gg

You can currently perform gg_obj + foo_obj as both objects inherit from gg and would produce the same +.gg method from Opt's double dispatch.

Currently, to control how the right-hand-side (foo objects) are added, we can define ggplot_add.foo.

Problem

However, currently we can not customize how left-hand-side objects (foo_obj + gg_obj) are added to gg objects.

Ex: left hand side structures are not ggplot2 objects and will not work with ggplot2 internal methods

x <- ggplot(mtcars, aes(wt, mpg)) + geom_point()
foo_obj <- structure(
 list(x, x), 
  class = c("foo", "gg")
)
# can not customize how the theme is added
foo_obj + theme_bw()

Ex: conflict + methods make addition impossible

x <- ggplot(mtcars, aes(wt, mpg)) + geom_point()
bar_obj <- structure(
 list(x, x), 
  class = c("bar") # does not inherit from 'gg'
)
`+.bar` <- function(e1, e2) {
  # only add to second plot
  e1[[2]] <- e1[[2]] + e2
  e1
}
# Conflicting `+` methods
bar_obj + theme_bw()
#> Error in bar_obj + theme_bw() : non-numeric argument to binary operator
#> In addition: Warning message:
#> Incompatible methods ("+.bar", "+.gg") for "+" 

Goal with PR

Define a add_gg.foo(e1, e2) method. When adding foo_obj to gg_obj, +.gg will be called, which will then call add_gg. add_gg will not invoke double dispatch (like + does), so custom left-hand-side methods can be created.

ggplot2's +.gg is not commutative. So I believe this situation has merit.

library(ggplot2)
x <- ggplot(mtcars, aes(wt, mpg)) + geom_point()
foo_obj <- structure(
 list(x, x), 
  class = c("foo", "gg") # does not inherit from 'gg'
)
`add_gg.foo` <- function(e1, e2) {
  # only add to second plot
  e1[[2]] <- e1[[2]] + e2
  e1
}
print.foo <- function(x, ...) {
  gridExtra::grid.arrange(x[[1]], x[[2]], ncol = 2)
}

foo_obj + theme_bw()

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

[thomasp85]

While I am not 100% against this, I’m very skeptic and fear that we will end up in a situation where the API clarity is impaired. So while it is a fairly simple change I think we should have some internal discussions about this before we move forward

[yutannihilation]

I'm against. I believe this should be solved by double dispatch, not by avoiding double dispatch. As I commented on #3815, it's possible even with the current ggplot2 if you wrap your object with S4. Anyway, a discussion about this is welcome.

[schloerke]

Agreed that it's possible to have it work with S4. One of the objects would be an S4 object, so S4 dispatch would occur when using an Ops method.

I can not upgrade my object to be S4 without breaking my downstream dependencies.

I don't know how this can be solved by double dispatch. The double dispatch that happens on all Ops methods (ex: +) is what is preventing me from making my own GGally::`+.ggmatrix` S3 method.

Being S3, it comes with the advantage that package developers can call NextMethod() when a situation should be handled by the later classes. (Rather than hard coding it to ggplot2's plus method.)

[yutannihilation]

I meant ggplot_add() should have been implemented to support double dispatch.

I can not upgrade my object to be S4 without breaking my downstream dependencies.

Can we see some examples of the breakage?

One more question, are there any due date to fix this issue? Even if this PR is accepted, the release would probably be 0.5 ~ 1 year away. I think GGally is one of the important packages in ggplot2 ecosystem so I believe we all want to help you solve the problem, but I'm afraid this PR might not be the best approach.

[schloerke]

Timeline: The ggplot2 v3.3.0 release got me moving again on GGally. But it has been this way for years, so waiting another 6-12 months is not the end of the world.

I'd rather have a correct solution than a rushed solution.

---

Off-hand, I believe plotly would break. Link of accessing p$nrow and p$ncol (and other p$* values): https://github.com/ropensci/plotly/blob/c47c1f957f1783d6212842463b0722c9473093b7/R/ggplotly.R#L86-L141

I am told users access information using the $ on their plot matrix as well.
(Note: I have not implemented the S4 change and run revdep check.)

[yutannihilation]

Off-hand, I believe plotly would break. Link of accessing p$nrow and p$ncol (and other p$* values)

Aw, I see... Thanks for the example.

[clauswilke]

@schloerke Could you open an issue for this topic? I feel having these discussions in the PR is not that useful. We should first properly document what the problem is, and then we can discuss different solutions to the problem.

[hadley]

While this fix isn't perfect, I think it's a reasonable hack to get the desired behaviour and it follows a similar pattern to what we've used in vctrs and I'm about to implement in R7.

[hadley]

@schloerke do you want to finish this off?

[schloerke]

@hadley Requested changes made. News entry added.

Please let me know if there is anything else I can add! Looking forward to R7!

[yutannihilation]

Let me clarify my stance on this pull request. Honestly, I'm not very happy that we are about to end up with exposing both add_ggplot() and ggplot_add(), which will probably confuse developers to some extent. I still believe this should be solved by utilizing double dispatch, but, considering we have failed to implement it for this 2 years (and hadley suggested it's not the time as R7 is not ready at the moment), I now admit this pull request is the realistic solution. Sorry @schloerke for having kept you waiting this long.

[thomasp85]

I think I can accept this if we rename the method to something else. I share @yutannihilation's feelings about having add_ggplot and ggplot_add as part of the exposed API.

A name I can live with and which makes the method very clear would be gg_add_dispatch(). It both describes what it does and looks sufficiently technical/ominous to dissuade people from messing with it without reason

[schloerke]

Providing an out here as well.... If R7 will solve this, I'm happy to wait for a proper solution. The current behavior is more inconvenient than it is broken. So no need to add to the API if it could be resolved later with R7.

[thomasp85]

If that is the case I vote for closing this and wait it out. @clauswilke and @yutannihilation are you fine with this?

[yutannihilation]

later with R7.

Just to be sure, "later" means "at least a few years away" (c.f. #4743 (comment)). I thought it would be a bit too far, so my take stays same as above, but I'm fine if everyone is fine.

[clauswilke]

No strong opinion either way from my side.

[emitanaka]

I'd prefer this sooner rather than later (especially if it's a few years down the road), but I don't have to deal with the aftermath of maintaining it, so I'm good with the decision that suits the team.