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

Drop all rows with empty slice() calls #6573

Merged
merged 3 commits into from
Nov 30, 2022

Conversation

DavisVaughan
Copy link
Member

@DavisVaughan DavisVaughan commented Nov 29, 2022

This revisits a point brought up in #2252

I went to remove the early exit in slice_rows() and discovered it was actually being used to ensure that slice() with no ... returns all rows. I think this is theoretically inconsistent.

I think that if we have:

df <- data.frame(x = 1:2)

then:

filter(df)

# Starts with this slice index
c(TRUE, TRUE)

# Uses `&` to combine it with the locations specified by `...`

# So it returns all rows

and:

slice(df)

# Starts with this slice index
integer()

# Uses `c()` to combine it with the locations specified by `...`

# So it returns no rows

This is inline with the logic mentioned here:
#2252 (comment)

And with these two examples:

df <- data.frame(x = 1:2)

slice(df, c())
#> [1] x
#> <0 rows> (or 0-length row.names)

slice(df, 1, 1)
#>   x
#> 1 1
#> 2 1

And it allows us to remove the early exit, which to me has always been an indication that the theoretical logic is sound

"e53f5c67-a3fa-4b29-93b0-4df2710f40fa"

@DavisVaughan DavisVaughan force-pushed the feature/slice-empty branch 2 times, most recently from b6668b7 to aace806 Compare November 29, 2022 19:15
@DavisVaughan
Copy link
Member Author

DavisVaughan commented Nov 29, 2022

I took a look through the 137 broken revdeps, and the only one that looks related to this is rsample, where I actually added some overly strict tests. I will go fix that, otherwise I think this is fine to merge

rsample pr: tidymodels/rsample#380

This allows us to remove the early exit, and is more theoretically consistent
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants