Skip to content
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 using functions that return NULL in aes() #2997

Merged

Conversation

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented Nov 13, 2018

It's common practice to wrap unsafe calculations with tryCatch() and return NULL, which allows us to use the result when available and skip when unavailable. But, currently, ggplot2 doesn't accept this in aes():

library(ggplot2)

df <- data.frame(x = 1:10, y = 1:10)
wrap <- function(...) tryCatch(..., error = function(e) NULL)

ggplot(df, aes(x, y, colour = wrap(no_such_column))) +
  geom_point()
#> Error: Aesthetics must be either length 1 or the same as the data (10): colour

This is because Layer$compute_aesthetics() removes NULL aes here BEFORE evaluation:

ggplot2/R/layer.r

Lines 221 to 223 in f5a88a7

aesthetics <- compact(aesthetics)
evaled <- lapply(aesthetics, rlang::eval_tidy, data = data)

Considering the following facts, I think it's safe to move compact() after evaluation

  • ggplot2::compact() removes only NULL
  • NULL will be still NULL after evaluation
@yutannihilation yutannihilation changed the title Accept null function Allow using functions that return NULL in aes() Nov 13, 2018
hadley
hadley approved these changes Nov 13, 2018
@@ -38,6 +38,13 @@ test_that("missing aesthetics trigger informative error", {
)
})

test_that("if an aes is mapped to a function that returns NULL, it is removed", {
df <- data.frame(x = 1:10)
wrap <- function(...) tryCatch(..., error = function(e) NULL)
Copy link
Member

@hadley hadley Nov 13, 2018

I think you can simplify this to just function() NULL

Copy link
Member Author

@yutannihilation yutannihilation Nov 13, 2018

Agreed.

@hadley
Copy link
Member

@hadley hadley commented Nov 13, 2018

Can you please add a bullet to NEWS? It should briefly describe the change and end with (@yourname, #issuenumber).

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Nov 13, 2018

Thanks! I tend to forget about NEWS bullets, sorry.

@yutannihilation yutannihilation merged commit b5dee5a into tidyverse:master Nov 13, 2018
4 checks passed
@yutannihilation yutannihilation deleted the accept-null-function branch Nov 13, 2018
@lock
Copy link

@lock lock bot commented May 13, 2019

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock bot locked and limited conversation to collaborators May 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants