Clearer error messages for invalid aesthetics #3091

clairemcwhite · 2019-01-19T22:00:36Z

Previously, if a stat wasn't valid for layers, the error message was confusing

# wrong, `fill` aesthetic mapped to stat-generated data column that
# doesn't exist in `geom_point()` layer
ggplot(mtcars, aes(mpg, 1, fill = stat(density))) +
  geom_tile(stat = "density") +
  geom_point(position = "jitter")
#> Error in is.finite(x): default method not implemented for type 'closure'

The new error message for this code is now

 #> Error: Aesthetics must be valid computed stats: fill. Did you map your stat in the wrong layer?

In the same vein, if a function like density was used as an aesthetic, the error message is clarified from the old:

# wrong, `fill` aesthetic mapped to data column that doesn't exist
ggplot(mtcars, aes(mpg, 1, fill = density)) +
  geom_tile(stat = "density") +
  geom_point(position = "jitter")
#> Don't know how to automatically pick scale for object of type function. Defaulting to continuous.
#> Error: Column `fill` must be a 1d atomic vector or a list

to the new error message below that prompts the user to use stat(density)

#> Don't know how to automatically pick scale for object of type function. Defaulting to continuous.
#> Error: Aesthetics must be valid data columns: fill. Did you forget to add stat()?

This was done by adding two checks in R/layer.r

For the first change:

    # Check that all columns in aesthetic stats are valid data
    nondata_stat_cols <- check_nondata_cols(stat_data)
    if(length(nondata_stat_cols) > 0){
      msg <- paste0(
        "Aesthetics must be valid computed stats: ", nondata_stat_cols,
        ". Did you map your stat in the wrong layer?"
      )
      stop(msg, call. = FALSE)
    }

and for the second change:

    nondata_cols <- check_nondata_cols(evaled) 
    if(length(nondata_cols) > 0){
      msg <- paste0(
        "Aesthetics must be valid data columns: ", nondata_cols,
        ". Did you forget to add stat()?"
      )
      stop(msg, call. = FALSE)
      }

Both use a new R/utilities.R function check_nondata_cols()

# This function checks that all columns of a dataframe `x` are data and
# returns the names of any columns that are not.
# We define "data" as atomic types or lists, not functions or otherwise
check_nondata_cols <- function(x) {
  idx <- (vapply(x, function(x) rlang::is_atomic(x) || rlang::is_list(x), logical(1)))
  names(x)[which(!idx)]
}

There are two test in tests/testthat/test-layer.r for these two situations


test_that("function aesthetics are wrapped with stat()", {
  df <- data_frame(x = 1:10)
  expect_error(
    ggplot_build(ggplot(df, aes(density)) + geom_tile(stat = "density")),
    "Aesthetics must be valid data columns:"
  )
})

test_that("computed stats are in appropriate layer", {
  df <- data_frame(x = 1:10)
  expect_error(
    ggplot_build(ggplot(df, aes(x = x, stat(density))) + geom_tile(stat = "density") + geom_point()),
    "Aesthetics must be valid computed stats:"
  )
})

Fixes tidyverse#3060

clauswilke · 2019-01-23T05:44:17Z

@thomasp85 Could you take a look at the code? It looks good to me but would be good to get a second pair of eyes on it.

@batpigandme Would you mind looking this over for the phrasing of the error messages?

thomasp85 · 2019-01-23T08:35:00Z

It appears to be good, but I can for the love of me not shake the feeling that it will error out in some fringe cases where it should not...

But I'm pretty sure that's just me — can't think of any instances where data could be other than atomic or recursive...

thomasp85 · 2019-01-23T08:35:38Z

@batpigandme if you are ok with the wording you can approve and merge

R/utilities.r

R/layer.r

hadley · 2019-01-23T15:02:49Z

R/layer.r

+    if(length(nondata_cols) > 0){
+      msg <- paste0(
+        "Aesthetics must be valid data columns: ", nondata_cols,
+        ". Did you forget to add stat()?"


Is forgetting stat() really the most likely cause?

There's two ways to get to this point: Forgetting stat() or mistyping a column name in such way that the result is a function, e.g. typing mean when the data column is called Mean. How about: "Did you mistype the name of a data column or forget to add stat()?"

Can we display the thing that they actually typed? Then we could hint to check that blah is a column in the data frame.

I don't know how to get it to print the actual typed statement, but now it prints the problematic aesthetic (along with the tilde).

Situation 1:
ggplot(mtcars, aes(x = mpg, stat(density))) + geom_tile(stat = "density") + geom_point()

Error: Aesthetics must be valid computed stats: ~stat(density). Did you map your stat in the wrong layer?

Situation 2:

ggplot(mtcars, aes(x = mpg, fill = density)) + geom_tile(stat = "density") + geom_point()

Don't know how to automatically pick scale for object of type function. Defaulting to continuous.
Error: Aesthetics must be valid data columns: ~density. Did you mistype the name of a data column or forget to add stat()?

clauswilke · 2019-01-23T17:09:48Z

It appears to be good, but I can for the love of me not shake the feeling that it will error out in some fringe cases where it should not...

I discussed this with @hadley at the developers day and the more I think about it the more I think it's right. Note that we're already performing a similar check on all incoming data, here:

ggplot2/R/utilities.r

Lines 440 to 444 in e9d4e5d

    
           # Check inputs with tibble but allow column vectors (see #2609 and #2374) 
        
           as_gg_data_frame <- function(x) { 
        
             x <- lapply(x, validate_column_vec) 
        
             new_data_frame(tibble::as_tibble(x)) 
        
           }

Though now that I think about it, maybe this check and the new check in this PR should be combined into one? Do we need to call tibble to validate the input data or could we simply run something like the check_nondata_cols() defined here?

thomasp85 · 2019-04-11T08:23:30Z

@clairemcwhite do you want to have a go at finishing this PR off?

clairemcwhite · 2019-04-11T12:43:24Z

I can take a shot this weekend

clauswilke · 2019-04-13T20:51:25Z

R/layer.r

+    nondata_cols <- check_nondata_cols(evaled)
+    if (length(nondata_cols) > 0) {
+      msg <- paste0(
+        "Aesthetics must be valid data columns: `", deparse(aesthetics[[nondata_cols]]),


I think it would be better to use as_label() instead of deparse().

clauswilke · 2019-04-13T20:51:33Z

R/layer.r

+    nondata_stat_cols <- check_nondata_cols(stat_data)
+    if (length(nondata_stat_cols) > 0) {
+      msg <- paste0(
+        "Aesthetics must be valid computed stats: `", deparse(aesthetics[[nondata_stat_cols]]),


clauswilke · 2019-04-13T20:55:24Z

As I said in the code comments, I think you should use as_label() instead of deparse(). However, be aware that there's a dependency with #3242, which isn't merged yet. For now, you should write rlang::as_label(). We can then wait merging this PR until #3242 is merged, and then change rlang::as_label() to as_label() here and merge this.

clairemcwhite · 2019-04-14T19:30:43Z

Deparse is now replaced with rlang::as_label. Current behavior is :

ggplot(mtcars, aes(x = mpg, stat(density))) + geom_tile(stat = "density") + geom_point()

Error: Aesthetics must be valid computed stats: stat(density). Did you map your stat in the wrong layer?

ggplot(mtcars, aes(x = mpg, fill = density)) + geom_tile(stat = "density") + geom_point()

Don't know how to automatically pick scale for object of type function. Defaulting to continuous.
Error: Aesthetics must be valid data columns: density. Did you mistype the name of a data column or forget to add stat()?

clauswilke

I've looked at everything carefully once more and saw a few more minor nitpicks (see code comments). Also, I'm still not 100% convinced by the error message. I feel it needs a better lead into the aesthetic names that are the problem. E.g.:

Error: Aesthetics must be valid computed stats. Problematic aesthetic(s): stat(density).

@batpigandme Could you comment?

Finally, Claire, you'll have to resolve the merge conflict before merging. You can do that by merging the current ggplot2 master into your branch.

NEWS.md

R/layer.r

clauswilke · 2019-04-14T22:50:44Z

Claire, could you check what happens when there are multiple problematic aesthetics in one layer? I just realized that that case may not work yet. It probably needs a paste0() and a vapply():

paste0(vapply(aesthetics[[nondata_stat_cols]], rlang::as_label, character(1)), collapse = ", ")

Also make sure the test functions check against the final text in the error message. You can probably just delete the final colon.

yutannihilation · 2019-04-24T01:22:18Z

#3242 is merged now.

…ics to resolve conflict Conflicts: NEWS.md R/layer.r

clauswilke · 2019-04-24T03:58:20Z

NEWS.md

@@ -81,6 +81,10 @@ core developer team.

 * ggplot2 now works in Turkish locale (@yutannihilation, #3011).

+* Clearer error messages for inappropriate aesthetics (@clairemcwhite, #3060).
+
+* `geom_rug()` gains an "outside" option to allow for moving the rug tassels to outside the plot area. (@njtierney, #3085)


Do we have an idea of where this news item comes from? It should have been present in NEWS.md already, but it isn't, so I'm confused about both why it was lost and how it made its reappearance here.

Ah, it was moved in this commit: 230e8f7 so you can just delete it here.

Got it, will do in next commit.

clauswilke · 2019-04-26T20:23:07Z

Claire, just a reminder, the deadline to merge this for the 3.2.0 release is April 30. It would be great if you could complete this by this deadline. Thanks!

clairemcwhite · 2019-04-27T21:53:02Z

@clauswilke

Multiple problem aesthetics now work.

Case 1:

ggplot(mtcars, aes(x = mpg, fill = density, color = density)) + geom_tile(stat = "density") + geom_point(stat = "density")
Don't know how to automatically pick scale for object of type function. Defaulting to continuous.
Don't know how to automatically pick scale for object of type function. Defaulting to continuous.
Error: Aesthetics must be valid data columns. Problematic aesthetic(s): density, density.
Did you mistype the name of a data column or forget to add stat()?

Case 2:

ggplot(mtcars, aes(x = mpg, fill = stat(density), colour = stat(density))) + geom_tile(stat = "density") + geom_point()
Error: Aesthetics must be valid computed stats. Problematic aesthetic(s): stat(density), stat(density).
Did you map your stat in the wrong layer?

The only improvement I can think of would be to have the error include the type of aesthetic, like: "Problematic aesthetics(s): fill = stat(density), color = stat(density)".

The variable "nondata_cols" is a vector c("fill", "color"), so there should be a quick way to construct this kind of error statement.

clauswilke · 2019-04-27T23:03:45Z

Very good! Final set of comments, I promise. :-)

To improve the error message, I think you can replace the function(x) {...} statement with:

function(x) {paste0(x, " = ", as_label(aesthetics[[x]]))}

I would use the case with multiple broken aesthetics in the unit tests, and check that the aesthetics are listed correctly (after addressing my point 1).
Do you need two geoms to create the multi-aesthetics case, or is one geom sufficient as long as you have aes(color = ..., fill = ...)?
What happens when both types of mistakes appear in the same plot?

clairemcwhite · 2019-04-28T18:03:19Z

Cool, the new function is function(x) {paste0(x, " = ", as_label(aesthetics[[x]]))}

Case 1:

ggplot(mtcars, aes(x = mpg, fill = density, color = density)) +  geom_point()
Don't know how to automatically pick scale for object of type function. Defaulting to continuous.
Don't know how to automatically pick scale for object of type function. Defaulting to continuous.
 Error: Aesthetics must be valid data columns. Problematic aesthetic(s): fill = density, colour = density. 
Did you mistype the name of a data column or forget to add stat()?

Case 2:

ggplot(mtcars, aes(x = mpg, fill = stat(density), color = stat(density))) + geom_point()
 Error: Aesthetics must be valid computed stats. Problematic aesthetic(s): fill = stat(density), colour = stat(density). 
Did you map your stat in the wrong layer?

One note is that this approach anglicizes my "color" to "colour"

I changed the tests to be more like what you suggested.
One geom is sufficient "ggplot(mtcars, aes(fill = stat(density), color = stat(density))) + geom_point()" and "ggplot(mtcars, aes(fill = density, color = density)) + geom_point()" produce the two errors
You get the errors in the order that they appear in layer.R. Remove the first error, and you get the second error

ggplot(mtcars, aes(x = mpg, fill = stat(density), color = density)) + geom_point()
Don't know how to automatically pick scale for object of type function. Defaulting to continuous.
 Error: Aesthetics must be valid data columns. Problematic aesthetic(s): colour = density. 
Did you mistype the name of a data column or forget to add stat()? 
# get rid of color = density
> ggplot(mtcars, aes(x = mpg, fill = stat(density))) + geom_point()
 Error: Aesthetics must be valid computed stats. Problematic aesthetic(s): fill = stat(density). 
Did you map your stat in the wrong layer?

clauswilke · 2019-04-28T18:05:23Z

Super, thanks! Assuming the automated regression tests pass I'll merge.

clauswilke · 2019-04-28T21:31:50Z

The AppVeyor build fails, for reasons that I can't discern. It doesn't seem to have anything to do with the changes that were made in this PR. The tests that break are visual tests for the date scale.

@thomasp85, any idea what's happening? Can we merge and hope for the best?

thomasp85 · 2019-04-29T08:01:34Z

It has been going on since 5aacacb... I think it is fair to assume it has nothing to do with this PR

If it propagates to master I'll fix it in a dedicated PR

clauswilke · 2019-04-29T14:37:31Z

Thanks!

lock · 2019-10-26T15:36:15Z

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/

clairemcwhite added 2 commits January 19, 2019 15:41

Clearer error messages for invalid aesthetics

99b4d05

Fixes tidyverse#3060

closes tidyverse#3060

078f650

clauswilke requested a review from batpigandme January 20, 2019 03:37

hadley reviewed Jan 23, 2019

View reviewed changes

thomasp85 added this to the ggplot2 3.2.0 milestone Apr 11, 2019

Modified error messages, fixed spacing, right line endings

33c953d

clairemcwhite force-pushed the clear_invalid_aesthetics branch from 39cf111 to 33c953d Compare April 13, 2019 20:12

clauswilke reviewed Apr 13, 2019

View reviewed changes

Replace use of deparse with rlang::as_label

aed6a9b

clauswilke requested changes Apr 14, 2019

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

R/layer.r Outdated Show resolved Hide resolved

R/layer.r Outdated Show resolved Hide resolved

R/layer.r Outdated Show resolved Hide resolved

Fix NEWS mispelling and punctuation. layer.R format suggestions

1ed5714

clairemcwhite added 3 commits April 23, 2019 21:40

Merge https://github.com/tidyverse/ggplot2 into clear_invalid_aesthet…

329d2c9

…ics to resolve conflict Conflicts: NEWS.md R/layer.r

Left in merge >>>, removed

6dbca4c

Remove punctuation in test-layer.R that was failing

81e45ea

clauswilke reviewed Apr 24, 2019

View reviewed changes

Add support for multiple problematic aesthetics, fix NEWS.md addition

5aacacb

clairemcwhite added 2 commits April 28, 2019 12:48

Change error to display whole problematic phrase, like fill = density

c2cc4d0

Update tests to be more specific

a789ac3

Fix failing tests, ideally

a7a70ae

clauswilke approved these changes Apr 28, 2019

View reviewed changes

clairemcwhite added 2 commits April 28, 2019 13:55

Another go at test passing

eb796a3

expect_error uses regex, so ( caused failing. Added fixed = TRUE

4f09ced

clauswilke merged commit 837e601 into tidyverse:master Apr 29, 2019

clauswilke mentioned this pull request Apr 30, 2019

AppVeyor tests failing #3279

Closed

lock bot locked and limited conversation to collaborators Oct 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clearer error messages for invalid aesthetics #3091

Clearer error messages for invalid aesthetics #3091

clairemcwhite commented Jan 19, 2019

clauswilke commented Jan 23, 2019

thomasp85 commented Jan 23, 2019

thomasp85 commented Jan 23, 2019

hadley Jan 23, 2019

clauswilke Jan 23, 2019

hadley Jan 23, 2019

clairemcwhite Apr 13, 2019

clauswilke commented Jan 23, 2019

thomasp85 commented Apr 11, 2019

clairemcwhite commented Apr 11, 2019

clauswilke Apr 13, 2019

clauswilke Apr 13, 2019

clauswilke commented Apr 13, 2019

clairemcwhite commented Apr 14, 2019

clauswilke left a comment

clauswilke commented Apr 14, 2019

yutannihilation commented Apr 24, 2019

clauswilke Apr 24, 2019

clauswilke Apr 24, 2019

clairemcwhite Apr 24, 2019

clauswilke commented Apr 26, 2019

clairemcwhite commented Apr 27, 2019

clauswilke commented Apr 27, 2019 •

edited

Loading

clairemcwhite commented Apr 28, 2019 •

edited

Loading

clauswilke commented Apr 28, 2019

clauswilke commented Apr 28, 2019

thomasp85 commented Apr 29, 2019 •

edited

Loading

clauswilke commented Apr 29, 2019

lock bot commented Oct 26, 2019

Clearer error messages for invalid aesthetics #3091

Clearer error messages for invalid aesthetics #3091

Conversation

clairemcwhite commented Jan 19, 2019

clauswilke commented Jan 23, 2019

thomasp85 commented Jan 23, 2019

thomasp85 commented Jan 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clauswilke commented Jan 23, 2019

thomasp85 commented Apr 11, 2019

clairemcwhite commented Apr 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clauswilke commented Apr 13, 2019

clairemcwhite commented Apr 14, 2019

clauswilke left a comment

Choose a reason for hiding this comment

clauswilke commented Apr 14, 2019

yutannihilation commented Apr 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clauswilke commented Apr 26, 2019

clairemcwhite commented Apr 27, 2019

clauswilke commented Apr 27, 2019 • edited Loading

clairemcwhite commented Apr 28, 2019 • edited Loading

clauswilke commented Apr 28, 2019

clauswilke commented Apr 28, 2019

thomasp85 commented Apr 29, 2019 • edited Loading

clauswilke commented Apr 29, 2019

lock bot commented Oct 26, 2019

clauswilke commented Apr 27, 2019 •

edited

Loading

clairemcwhite commented Apr 28, 2019 •

edited

Loading

thomasp85 commented Apr 29, 2019 •

edited

Loading