mutate() doesn't need to calculate for empty groups

[yutannihilation]

The verbs that returns the same numbers of rows as the input (e.g. mutate(), filter()) probably doesn't need to calculate the empty groups because they never appear in the result. Actually, filter() and arrange() seem to skip empty groups, but mutate() doesn't:

library(dplyr, warn.conflicts = FALSE)

f <- function(x) {
  if (length(x) == 0) {
    warning("x is empty", call. = FALSE)
    return(NA)
  }
  x
}

# empty groups are not calculated
d <- data.frame(x = factor(1, levels = 1:3)) %>%
  group_by(x) %>%
  filter(is.na(f(x)))

# empty groups are calculated
d <- data.frame(x = factor(1, levels = 1:3)) %>%
  group_by(x) %>%
  mutate(foo = f(x))
#> Warning: x is empty

#> Warning: x is empty

^{Created on 2019-01-08 by the reprex package (v0.2.1)}

Skipping empty groups is not only good for performance, but also nice to avoid warnings or errors related to 0-length vectors. I feel the warnings above seem a bit puzzling.

Maybe Gatherer can check indices.size() around here?

dplyr/src/mutate.cpp

Lines 272 to 276 in ce7f2bc

	for (; i < ngroups; i++, ++git) {
	const Index& indices = *git;
	Shield<SEXP> subset(proxy.get(indices));
	grab(subset, indices);
	}

But, of course, summarise() cannot skip empty groups. This means Gatherer needs to know the context whether the operation is for summary or not to determine whether to skip the calculation or not. So, this might make the implementation a bit complex.

I think this problem is not so serious as #4061 because I expect this doesn't break the existing code so hard, but it's nice if it is possible to avoid unnecessary computations.

[romainfrancois]

Yes, thanks for pointing that out, I thought mutate() did not evaluate the expressions on empty groups.

[romainfrancois]

summarise() does not use Gatherer

[krlmlr]

I'm a bit torn here. What if evaluation of groups had a desired side effect? Maybe we could decide this based on the new .drop argument?

[romainfrancois]

mutate() is probably not where to think about side effects.

I don't understand the point about .drop, as if you did .drop = TRUE there are no empty groups in the first place.

[hadley]

@romainfrancois this probably needs a test for when all groups are empty. What type does the generated column have?

[romainfrancois]

Turns out in that case, this lines gets it:

https://github.com/tidyverse/dplyr/blob/master/src/mutate.cpp#L168

library(dplyr, warn.conflicts = FALSE)

d <- tibble(f = factor(levels = c("a", "b"))) 
d %>% 
  group_by(f) %>% 
  mutate(x = print(32.5))
#> [1] 32.5
#> # A tibble: 0 x 2
#> # Groups:   f [2]
#> # … with 2 variables: f <fct>, x <dbl>

but it might be better to handle it with the case where there are no groups:

dplyr/src/mutate.cpp

Line 490 in 1c474b5

if (gdf.ngroups() == 0) {

[yutannihilation]

summarise() does not use Gatherer

Oh, thanks for the correction! Now I'm not sure know why I assumed so...

[yutannihilation]

@krlmlr Do you have any actual examples in your mind? I've come up with this concern, but I couldn't find any useful cases. For side effects, I think group_walk() is enough and should be used.

What if evaluation of groups had a desired side effect?

[krlmlr]

Just my own FUD here. I think it's fine if we document the behavior, also with a test.

[yutannihilation]

Thanks, agreed.

[lock]

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/