-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Allow empty annotations. #3320
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 empty annotations. #3320
Conversation
dad6dc1
to
cfd4962
Compare
@hadley @yutannihilation @thomasp85 I would need a formal approval before I can merge this. It was Ok'd informally by Thomas here: #3317 (comment) |
tests/testthat/test-performance.R
Outdated
@@ -1,5 +1,8 @@ | |||
context("Performance related alternatives") | |||
|
|||
# ******************** | |||
# modify_list() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please use our standard sectioning convention?
# modify_list() ----------------------------------------
R/annotation.r
Outdated
unequal <- length(unique(setdiff(lengths, 1L))) > 1L | ||
if (unequal) { | ||
n <- unique(setdiff(lengths, 1L)) | ||
if (length(n) == 0L) n <- 1L # if all lengths are equal to 1L then above line fails, this fixes that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it fail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a poorly formulated comment. The purpose of line 49 is to find the unique lengths that are not 1L
. However, if lengths == c(1L)
, then we get n <- numeric(0)
in line 49, and we need n <- 1L
in this case.
R/annotation.r
Outdated
# To determine the final number of rows `n` in the data frame, | ||
# we need to find the unique lengths not equal to 1L (and there | ||
# should be at most one such length). However, if all lengths | ||
# are equal to 1L, then the final number of rows is also 1L. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this would be a clearer approach?
ulength <- unique(lengths)
unequal <- length(ulength) > 1 || ulength != 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that doesn't capture it. unequal
should only be true if there is more than one length excluding 1. So if lengths <- c(1, 2)
, we should have unequal <- FALSE
, but if lengths <- c(2, 3)
, we should have unequal <- TRUE
. I'll see if I can find a better way to code this.
The original statement to calculate unequal
was exactly right:
unequal <- length(unique(setdiff(lengths, 1L))) > 1L
But now I'm also trying to capture the length n
in addition, that required the additional code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've thought about it some more, and I can't come up with something that would be clearer and simpler than the code in the current PR. Again, we also need to capture n
, since it's used in line 64:
Line 64 in 4ff8243
data <- new_data_frame(position, n = n) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I woke up this morning, I realized that using setdiff()
only when needed may make the code easier to understand. I pushed a new version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's much easier to understand — thanks!
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/ |
This fixes #3317, and likely some other current issues.
Created on 2019-05-08 by the reprex package (v0.2.1)