Skip to content

Annotations with zero-length aesthetics are broken #3317

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

Closed
paleolimbot opened this issue May 8, 2019 · 10 comments · Fixed by #3320
Closed

Annotations with zero-length aesthetics are broken #3317

paleolimbot opened this issue May 8, 2019 · 10 comments · Fixed by #3320
Milestone

Comments

@paleolimbot
Copy link
Member

paleolimbot commented May 8, 2019

In the revdep checks (#3303), an issue was identified where zero-length aesthetics in annotate() caused newly broken plots (interflex package).

library(ggplot2)
ggplot() +
  annotate("point", x = numeric(0), y = numeric(0), colour = "red")
#> Error: Elements must equal the number of rows or 1

In 3.1.1, this did not error:

library(ggplot2)
ggplot() +
  annotate("point", x = numeric(0), y = numeric(0), colour = "red")

@clauswilke
Copy link
Member

This and possibly some of the other problems could be solved if new_data_frame() returned an empty data frame in case some columns have length zero. I think that's reasonable behavior. See examples.

library(ggplot2)

# empty data frame creates empty plot
df <- data.frame(x = numeric(0), y = numeric(0))
ggplot(df, aes(x, y)) + geom_point()

# data.frame() throws an error in this case; this is what we're currently mirroring
data.frame(x = numeric(0), y = numeric(0), z = "red")
#> Error in data.frame(x = numeric(0), y = numeric(0), z = "red"): arguments imply differing number of rows: 0, 1

# however, tibble() does not throw an error; I think this behavior is reasonable
tibble::tibble(x = numeric(0), y = numeric(0), z = "red")
#> # A tibble: 0 x 3
#> # … with 3 variables: x <dbl>, y <dbl>, z <chr>

Created on 2019-05-08 by the reprex package (v0.2.1)

@thomasp85 Any concerns about the proposed solution?

clauswilke added a commit to wilkelab/ggplot2_archive that referenced this issue May 8, 2019
This was referenced May 8, 2019
@thomasp85
Copy link
Member

No concerns in general, other than the possibility of hard-to-debug bugs

@clauswilke
Copy link
Member

I'm a big proponent of functions that behave gracefully when they're given empty input, and I find that many frustrating bugs are actually caused by functions that insist they receive non-empty input. If I remember correctly, @lionel- once quoted some fancy LISP principle to this effect, but I don't remember the details.

@thomasp85
Copy link
Member

I agree, but as I understand it this is not about zero input. This is about multiple inputs among which some are zero, right?

@clauswilke
Copy link
Member

It's about multiple inputs when some are zero and others are constants (length 1). If some are zero and others are length > 1 you still get an error. This mirrors exactly what tibble is doing. (See also here: https://tibble.tidyverse.org/articles/tibble.html#recycling)

ggplot2:::data_frame(x = numeric(0), color = character(0))
#> [1] x     color
#> <0 rows> (or 0-length row.names)
ggplot2:::data_frame(x = numeric(0), y = 1, color = "red")
#> [1] x     y     color
#> <0 rows> (or 0-length row.names)
ggplot2:::data_frame(x = numeric(0), y = 1, color = c("red", "blue"))
#> Error: Elements must equal the number of rows or 1

tibble::tibble(x = numeric(0), color = character(0))
#> # A tibble: 0 x 2
#> # … with 2 variables: x <dbl>, color <chr>
tibble::tibble(x = numeric(0), y = 1, color = "red")
#> # A tibble: 0 x 3
#> # … with 3 variables: x <dbl>, y <dbl>, color <chr>
tibble::tibble(x = numeric(0), y = 1, color = c("red", "blue"))
#> Tibble columns must have consistent lengths, only values of length one are recycled:
#> * Length 0: Column `x`
#> * Length 2: Column `color`

Created on 2019-05-11 by the reprex package (v0.2.1)

@lionel-
Copy link
Member

lionel- commented May 13, 2019

once quoted some fancy LISP principle to this effect, but I don't remember the details.

Was it nil punning? That's a bit different from recycling to zero (which might be viewed as empty punning). Aren't lisp principles quaint rather than fancy though ;)

I agree the issue seems to be about recycling to zero.

@thomasp85
Copy link
Member

As I said, I don’t have a big stake in this and we can change how it works if that makes everything else work. I was just foreseeing that this would lead to some hard-to-debug errors at some point🙂

@clauswilke
Copy link
Member

Lionel: Yes, that's what I meant.

Thomas: Could you take a look at the actual PR to make sure it's Ok and doesn't degrade performance? #3320

@thomasp85
Copy link
Member

There shouldn’t be any meaningful degradation of performance, AFAIK

clauswilke added a commit that referenced this issue May 16, 2019
* allow empty annotations. fixes #3317

* fix corner case where all annotation parameters have length 1

* add unit tests, simplify code; closes #3314

* improve comments

* simplify code calculating number of rows in annotation data frame
@lock
Copy link

lock bot commented Nov 12, 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 Nov 12, 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 a pull request may close this issue.

4 participants