Skip to content

Allow using functions that return NULL in aes() #2997

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

Merged

Conversation

yutannihilation
Copy link
Member

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
@@ -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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

@hadley
Copy link
Member

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

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

@yutannihilation yutannihilation merged commit b5dee5a into tidyverse:master Nov 13, 2018
@yutannihilation yutannihilation deleted the accept-null-function branch November 13, 2018 23:08
@lock
Copy link

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
Development

Successfully merging this pull request may close these issues.

2 participants