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

filter() silently accepting matrices #5973

Closed
romainfrancois opened this issue Aug 6, 2021 · 3 comments · Fixed by #5974
Closed

filter() silently accepting matrices #5973

romainfrancois opened this issue Aug 6, 2021 · 3 comments · Fixed by #5974

Comments

@romainfrancois
Copy link
Member

romainfrancois commented Aug 6, 2021

library(dplyr, warn.conflicts = FALSE)

df <- data.frame(x = 1:2, y = 3:4)

this fails, as it should

filter(df, c(TRUE, TRUE, FALSE, FALSE))
#> Error: Problem with `filter()` input `..1`.
#> ℹ Input `..1` is `c(TRUE, TRUE, FALSE, FALSE)`.
#> x Input `..1` must be of size 2 or 1, not size 4.

but this does not fail, and worse it silently uses the first column of the matrix:

filter(df, matrix(c(TRUE, TRUE, TRUE, FALSE), nrow = 2))
#>   x y
#> 1 1 3
#> 2 2 4

however if we convert the matrix to a data frame, then
we get an auto splicing behavior

filter(df, as.data.frame(matrix(c(TRUE, TRUE, TRUE, FALSE), nrow = 2)))
#>   x y
#> 1 1 3

Created on 2021-08-06 by the reprex package (v2.0.0)

@romainfrancois
Copy link
Member Author

This is related to #5972

@romainfrancois
Copy link
Member Author

romainfrancois commented Aug 6, 2021

we already have plans for disabling, or at least discouraging data frame results inside filter(), which were only useful before we had if_any() and if_all():

    } else if(Rf_inherits(res, "data.frame")) {
      // disable the warnings entirely for now, so that we can first release
      // if_any() and if_all(), will start warning later
      bool warn = false;

      if (first && warn) {
        SEXP expr = rlang::quo_get_expr(VECTOR_ELT(quos, i));
        bool across = TYPEOF(expr) == LANGSXP && CAR(expr) == dplyr::symbols::across;
        if (across) {
          Rf_warningcall(R_NilValue, "Using `across()` in `filter()` is deprecated, use `if_any()` or `if_all()`");
        } else {
          Rf_warningcall(R_NilValue, "data frame results in `filter()` are deprecated, use `if_any()` or `if_all()`");
        }
      }

      const SEXP* p_res = VECTOR_PTR_RO(res);
      R_xlen_t ncol = XLENGTH(res);
      for (R_xlen_t j = 0; j < ncol; j++) {
        reduce_lgl_and(reduced, p_res[j], n);
      }
    }

Not sure what to do with matrices ? Forbid them ? Treat them as data frames ?

This feels like a reach for natural use of filter() but the matrix might come from some other function, as in #5972 where we get a matrix by applying is.na() to a data frame:

library(dplyr, warn.conflicts = FALSE)

tibble(
  y = tibble(
    b = c(1, 2),
    a = c(NA, 1),
  )
) %>% 
  filter(is.na(y))
#> # A tibble: 0 x 1
#> # … with 1 variable: y <tibble[,2]>

Created on 2021-08-06 by the reprex package (v2.0.0)

@romainfrancois
Copy link
Member Author

relatedly, slice() just lets matrices pass:

library(dplyr, warn.conflicts = FALSE)

df <- data.frame(x = 1:2)
slice(df, matrix(c(1, 2, 1, 2), nrow = 2))
#>   x
#> 1 1
#> 2 2
#> 3 1
#> 4 2

Created on 2021-08-06 by the reprex package (v2.0.0)

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 a pull request may close this issue.

1 participant