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

Revisit definition of slice(df) #2252

Closed
krlmlr opened this issue Nov 12, 2016 · 4 comments
Closed

Revisit definition of slice(df) #2252

krlmlr opened this issue Nov 12, 2016 · 4 comments
Assignees
Labels
feature a feature request or enhancement
Milestone

Comments

@krlmlr
Copy link
Member

krlmlr commented Nov 12, 2016

Currently returns all rows, but it's not tested anywhere. I think it should return an empty data frame instead, and slice(df, i1, i2) should be the same as slice(df, c(i1, i2)).

@hadley: Thoughts?

@hadley
Copy link
Member

hadley commented Nov 12, 2016

I think the motivation was for filter(x) and slice(x) to give identical results, but maybe that's wrong because an empty logical should be treated as selecting all rows, whereas an empty integer should be treated as selecting no row.

Equally, filter() ANDs together the inputs, and here you're proposing (effectively) that slice OR's together the inputs.

@krlmlr
Copy link
Member Author

krlmlr commented Nov 12, 2016

Yes, that's the way I'd see it -- perhaps not even OR, but UNION ALL. FWIW, slice(df, i1, i2) currently throws an error.

@hadley hadley added the feature a feature request or enhancement label Jan 31, 2017
@hadley
Copy link
Member

hadley commented Feb 22, 2017

Can you tackle this one too please? I think it should be a one-line change in slice_impl().

@krlmlr krlmlr added this to the data frame 2 milestone Feb 22, 2017
@krlmlr
Copy link
Member Author

krlmlr commented Mar 6, 2017

I have now found the test. It seems that slice() intentionally accepts either 0 or one args, negative values are treated as exclusions. The current behavior actually makes some sense, I see no reason to change it.

We could still consider supporting multiple arguments to slice(), but only after #2311.

@krlmlr krlmlr closed this as completed Mar 6, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jun 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature a feature request or enhancement
Projects
None yet
Development

No branches or pull requests

2 participants