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

group_by(.drop =) #4091

Merged
merged 33 commits into from
Jan 30, 2019
Merged

group_by(.drop =) #4091

merged 33 commits into from
Jan 30, 2019

Conversation

romainfrancois
Copy link
Member

@romainfrancois romainfrancois commented Jan 8, 2019

This is not ready yet, I've only so far just added the drop argument, it currently does nothing useful.

@romainfrancois romainfrancois added the wip work in progress label Jan 8, 2019
@romainfrancois
Copy link
Member Author

closes #4061

@romainfrancois
Copy link
Member Author

I've tried to follow .drop around (joins, implicit group by, count, ...) but I might be missing a few places.

it is stored as an attribute of the grouping structure

library(dplyr)
#> 
#> Attaching package: 'dplyr'
#> The following objects are masked from 'package:stats':
#> 
#>     filter, lag
#> The following objects are masked from 'package:base':
#> 
#>     intersect, setdiff, setequal, union

tibble(
  x = 1:2, 
  f = factor(c("a", "b"), levels = c("a", "b", "c"))
) %>% 
  group_by(f, .drop = TRUE) %>% 
  group_data() %>% 
  str()
#> Classes 'tbl_df', 'tbl' and 'data.frame':    2 obs. of  2 variables:
#>  $ f    : Factor w/ 3 levels "a","b","c": 1 2
#>  $ .rows:List of 2
#>   ..$ : int 1
#>   ..$ : int 2
#>  - attr(*, ".drop")= logi TRUE

The unexported group_drops() says if the data drops.

Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overall structure looks good, but someone else needs to do a careful reading of the code. @lionel-?

NEWS.md Outdated Show resolved Hide resolved
@@ -6,7 +6,8 @@
#'
#' @family grouping functions
#' @inheritParams scoped
#' @param .add Passed to the `add` argument of [group_by()].
#' @param .add,.drop see [group_by()]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just @inheritParams these?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 it's add on group_by() and .add on group_by_*() I can :

  • document .add here (probably, but that's kind of bad)
  • switch to .add in group_by() (probably not)
  • switch to add in group_by_*() (I guess not either)

@lionel-
Copy link
Member

lionel- commented Jan 24, 2019

I'll take a look tomorrow.

Copy link
Member

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good. I'll try running this PR against the tests as defined in v0.7.8, hope there are now few failing tests if any.

R/count-tally.R Outdated Show resolved Hide resolved
R/count-tally.R Outdated Show resolved Hide resolved
R/group-by.r Show resolved Hide resolved
R/group-by.r Outdated Show resolved Hide resolved
R/group-by.r Show resolved Hide resolved
inst/include/dplyr/data/GroupedDataFrame.h Outdated Show resolved Hide resolved
tests/testthat/test-group-by.r Outdated Show resolved Hide resolved
tests/testthat/test-count-tally.r Show resolved Hide resolved
tests/testthat/test-group_data.R Outdated Show resolved Hide resolved
R/grouped-df.r Outdated Show resolved Hide resolved
NEWS.md Outdated Show resolved Hide resolved
@romainfrancois
Copy link
Member Author

Should group_by have both .add and add for some time and then we slowly remove add ?

group_by <- function(.data, ..., add = FALSE, .add = add, .drop = FALSE) {
  ...
}

@hadley
Copy link
Member

hadley commented Jan 29, 2019

@romainfrancois we should tackle that in a separate PR, and it can wait until 0.9.0.

@romainfrancois romainfrancois merged commit 160b349 into master Jan 30, 2019
@romainfrancois romainfrancois deleted the group_by_dot_drop branch January 30, 2019 14:42
wch pushed a commit to rstudio/shinyloadtest that referenced this pull request May 13, 2019
* add helper methods to work around dev dplyr bug

tidyverse/dplyr#4096

* use dplyr PR that fixes group_by bug

Depending on tidyverse/dplyr#4091 to be merged

* use new `.drop = TRUE` argument for `group_by` and remove all `convert_*` methods.

* use ungroup and group(run = droplevels(run)) or similar where appropriate

works with dplyr 0.7.8 and 0.8.0

* use cran dplyr >= v0.8.0.1 and not the remote

* remove dplyr.R file

* white space

* undo unnecessary changes

* remove notion of  from plotting.R

* clean up run = run in group_by
@lock
Copy link

lock bot commented Jul 29, 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 Jul 29, 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 this pull request may close these issues.

4 participants