New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow geoms to ignore some set aesthetics #1585

Closed
jiho opened this Issue Mar 14, 2016 · 21 comments

Comments

Projects
None yet
5 participants
@jiho
Contributor

jiho commented Mar 14, 2016

Let us say I want to combine a few geoms to make a specific plot, I can define such a function:

library("ggplot2")

d <- data.frame(x=rnorm(10), y=rnorm(10), a=letters[1:10])

my_plot <- function(data, mapping, ...) {
  ggplot() +
    geom_line(data=data, mapping=mapping, ...) +
    geom_point(data=data, mapping=mapping, ...)
}

my_plot(d, aes(x=x, y=y))

But then, if I want to change the linetype, geom_point throws an error because linetype is meaningless for it:

my_plot(d, aes(x=x, y=y), linetype="dashed")
Error: Unknown parameters: linetype

If I recall correctly, this was just silently ignored in the past. Interestingly, mapped aesthetics do not throw this error:

my_plot(d, aes(x=x, y=y, linetype="dashed"))

But the result is pretty useless of course ;-). A more useful example would be

my_plot <- function(data, mapping, ...) {
  ggplot() +
    geom_line(data=data, mapping=mapping, ...) +
    geom_point(data=data, mapping=mapping, ...) +
    geom_text(data=data, mapping=mapping, ...)
}
my_plot(d, aes(x=x, y=y, label=a))

Still moving the text up is not possible after the fact, with something like:

my_plot(d, aes(x=x, y=y, label=a), vjust=0)

I understand the rationale in telling the user explicitly about useless aesthetics, but:

  • I think throwing an error is a bit too much, a warning would be enough. Stopping the execution of the function makes it very cumbersome to use ggplot programmatically to build new visualisations (the "proper" way may be to build new geoms but this requires a lot of sophistication for a simple goal).
  • Alternatively (or in addition) there should be a way to easily bypass the warning/errors, through an argument, FALSE by default (if a warning is used, the person creating the function can also wrap the call in suppressWarnings and that would help).
  • This should be consistent between set and mapped aesthetics (but probably ignore useless inherited aesthetics without warning).

PS: The rationale for this comment is that I often combine geoms in autoplot but want to retain the flexibility of mapping/setting aesthetics from the higher level function.

@faizan-khan-iit

This comment has been minimized.

faizan-khan-iit commented Jun 13, 2016

I also run into the same error when I try to run this example:

library("ggplot2")

viz <- list(iris=ggplot()+
    geom_point(aes(Petal.Width, Sepal.Length, showSelected=Species),
               data=iris, chunk_vars=character()))
Error: Unknown parameters: chunk_vars

This does enforce the point that spelling mistakes and other minor errors will be easily caught and avoided. But this also breaks the packages like Animint, that rely on the use additional parameters (for e.g. chunk_vars in the above example)

I agree with @jiho in that we should be able to bypass this error by specifying a parameter so that we may only get a warning about additional params. It will ensure that the existing packages work with little modifications, all the while achieving your intended goal of not allowing user to make minor mistakes.

I hope you give this a thought.

@tdhock

This comment has been minimized.

tdhock commented Aug 1, 2016

Have you decided how to address this issue? Is there a PR? Or has the feature already been implemented?

@thomasp85

This comment has been minimized.

Member

thomasp85 commented Aug 2, 2016

@hadley how do you want this? An ignore.extra argument set to FALSE by default which would make the layer silently ignore extra parameters when switched on?

@hadley

This comment has been minimized.

Member

hadley commented Aug 2, 2016

I think there are two parts to the problem:

  • Informing ggplot2 about new aesthetics. I think a reasonable solution here would be to provide an API for packages to register aesthetics - i.e. on load, a animInt would call ggplot2::register_aesthetic("chunk_vars")).
  • Making it easier to construct multilayer plots with varying aesthetics. Here an ignore.extra parameter to the layer seems reasonable. The problem is that adding a new parameter to layer() involves also adding it to every layer function (e.g. geom_point(), geom_histogram(), ... So I'd like to think about alternative approaches

I don't like the idea of changing the error to a warning because it doesn't fix the underlying problem.

@tdhock

This comment has been minimized.

tdhock commented Aug 2, 2016

or maybe ggplot2::register_param("chunk_vars")? in animint chunk_vars is a parameter, not an aesthetic. For example geom_point(aes(x=foo, y=bar, showSelected=baz), chunk_vars=character())

@tdhock

This comment has been minimized.

tdhock commented Aug 2, 2016

another alternative would be to introduce a new parameter for storing any/all arbitrary layer-specific meta-data.

for example geom_point(aes(x=foo, y=bar, showSelected=baz), meta=list(chunk_vars=character()))

the desired behavior would be that ggplot2 will recognize the meta parameter, store that list, and NOT stop with an error.

@thomasp85

This comment has been minimized.

Member

thomasp85 commented Aug 2, 2016

I think the discussion has now diverged som much from the original (ignoring unknown aesthetics) that it warrants its own issue...

@hadley

This comment has been minimized.

Member

hadley commented Aug 2, 2016

Yes, let's move the extra parameters to a separate issue. It's not clear to me that you should be allowed to add extra parameters to a geom that you didn't create.

@thomasp85

This comment has been minimized.

Member

thomasp85 commented Aug 2, 2016

Can't really understand the need either, but let's take that discussion in the new issue. @tdhock if you decide to pursue this please provide a thorough description of a use case. It is not clear from your code what chunk_vars is supposed to do as geom_point don't know how to utilise the parameter

@thomasp85

This comment has been minimized.

Member

thomasp85 commented Aug 2, 2016

@hadley are you dead-set on erroring out by default? Otherwise it could be solved with removing a couple of lines of code...

@hadley

This comment has been minimized.

Member

hadley commented Aug 2, 2016

I don't know if I'm dead-set against warnings, but an error certainly seems to better match the semantics of calling a function with an extra argument. I'd like to thoroughly explore alternatives before committing to a warning.

@thomasp85

This comment has been minimized.

Member

thomasp85 commented Aug 2, 2016

I was more thinking about silently ignoring extra aesthetics. Agree that a warning is pretty useless.

@hadley

This comment has been minimized.

Member

hadley commented Aug 2, 2016

You mean using the list of aesthetics in .all_aesthetics?

@thomasp85

This comment has been minimized.

Member

thomasp85 commented Aug 2, 2016

I'm naively thinking about deleting line 102-106 in layer.r

@hadley

This comment has been minimized.

Member

hadley commented Aug 2, 2016

Oh you want to move completely back to the old experience where there's no warning or error if you mistype an aesthetic name?

@jiho

This comment has been minimized.

Contributor

jiho commented Aug 2, 2016

I think the distinction between parameter (i.e. a set aesthetic) or and aesthetic (i.e. a mapped aesthetic, inside an aes() call) is moot. A geom has a set of valid aesthetics (in the broad sense) and if one is mis-spelled, either in aes() or outside of it, the user should be informed about it. In that sense, even the current behaviour is broken, i.e. this works without a warning/error:

library("ggplot2")
ggplot(mtcars) + geom_point(aes(x=mpg, y=disp, sahpe=factor(cyl)))

when in fact I want

ggplot(mtcars) + geom_point(aes(x=mpg, y=disp, shape=factor(cyl)))

Once this is solved, then the problem of passing additional information to geoms would arise for both set and mapped aesthetics. For this, I really think the way to go is a warning. You want to let the user know he/she added an argument that, maybe, won't do anything (e.g. adding linetype for geom_point). The result will not be invalid, so it should not be an error; it may be not exactly what the user wanted and warning him/her about it is nice (but the previous behaviour of silently ignoring it was also valid; it actually is what most functions accepting ... as argument do in my experience).

So to take my initial example again:

d <- data.frame(x=rnorm(10), y=rnorm(10), a=letters[1:10])

library("ggplot2")
my_plot <- function(data, mapping, ...) {
  ggplot() +
    geom_line(data=data, mapping=mapping, ...) +
    geom_point(data=data, mapping=mapping, ...)
}

I would expect both these lines:

my_plot(d, aes(x=x, y=y, linetype=a))
my_plot(d, aes(x=x, y=y), linetype="dashed")

to trigger a warning such as:

Warning message:
'linetype' is not a valid aesthetic for geom_point and will be ignored.

If you want to go out of your way to be helpful to the user, you might even make a partial match between the valid aesthetics and the provided ones to detect typos and provide helpful warnings such as

library("ggplot2")
ggplot(mtcars) + geom_point(aes(x=mpg, y=disp, sahpe=factor(cyl)))
Warning message:
'sahpe' is not a valid aesthetic for geom_point. Maybe you meant 'shape'?

git or apt-get do this. It could be quite easy to implement something based on the Levenshtein distance. I have code to compute it.

I see only one exception: inherited aesthetics. You don't want

ggplot(mtcars, aes(x=mpg, y=disp, shape=factor(cyl)) + geom_point() + geom_path()

to emit a warning about shape not being valid for geom_path. It would be cumbersome and would defeat the purpose of inheritance.

So basically, either of these solutions would be fine with me:

  • delete lines 102-106 in layers.r and silently ignore all extra stuff (which was the previous behaviour if I am not mistaken)
  • change how mapped aesthetics (in aes()) are dealt with and detect non-valid ones just like extra parameters are detected. In both cases, warn about unknown ones (except for inherited aesthetics).
@thomasp85

This comment has been minimized.

Member

thomasp85 commented Aug 2, 2016

I'll let it be up to you @hadley, one of the bullets is definitely easier ;-)

@hadley

This comment has been minimized.

Member

hadley commented Aug 2, 2016

I agree that aesthetics should be treated identically regardless of whether or not they're set or mapped. Silently ignoring unknown aesthetics is definitely bad (regardless of whether or not that's standard behaviour for functions that use ...). That can be fixed in layer.r by comparing geom$aesthetics() with names(mapping), and we should do that regardless of the other changes.

It would be nice to automatically suggest correct names using utils::adist().

I worry about emitting warnings rather than throwing errors because it means that you'd expect functions like my_plot() to routinely throw warnings - that doesn't seem like a good design to me.

@jiho

This comment has been minimized.

Contributor

jiho commented Aug 3, 2016

I may have a stab at it if I can find the time (doing field work now, with a pretty crazy schedule, so not sure when).

re- the warning vs. error situation. I suggested a warning because, in a function like my_plot() above, that would allow me to use suppressWarnings, which acknowledges the fact that there are warnings but let it still work, while there is, obviously, no suppressErrors. An alternative already mentioned would be an argument ignore.extra set to FALSE by default but this seemed problematic (need to add it to all geoms) and does not really change much conceptually compared to suppressWarnings.

@hadley

This comment has been minimized.

Member

hadley commented Sep 23, 2016

In an ideal world, ggplot2 would have throw errors to avoid these problems from the get go. But it seems like throwing errors now is a bit too harsh — I don't think warnings are as good, but they're a reasonable compromise given code in the wild.

I'll have a go at making the change.

@hadley hadley closed this in befc4d0 Sep 23, 2016

@jiho

This comment has been minimized.

Contributor

jiho commented Sep 25, 2016

That's great, thanks!
(The change wasn't so trivial in the end)

@lock lock bot locked as resolved and limited conversation to collaborators Jun 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.