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

keep() and discard() err on NA ... perhaps unintentionally? #1156

Open
mmuurr opened this issue Dec 10, 2024 · 0 comments
Open

keep() and discard() err on NA ... perhaps unintentionally? #1156

mmuurr opened this issue Dec 10, 2024 · 0 comments

Comments

@mmuurr
Copy link

mmuurr commented Dec 10, 2024

I haven't checked previous {purrr} versions to see if this is new(ish) behavior, but for some reason this surprised me:

keep(c(TRUE, FALSE, NA), identity)
# Error in `keep()`:
# ℹ In index: 3.
# Caused by error:

(Same for discard().)

When inspecting keep(), however, we see that:

keep
# function(.x, .p, ...) {
#   where <- where_if(.x, .p, ...)
#   .x[!is.na(where) & where]
# }

... suggesting that it's intended to handle NA values (same for discard()), but it's purrr:::where_if() that's actually throwing the error.

Is this intended?

While I tend to try to opt for explicitness as much as possible, I think the NA-handling as coded in keep() and discard() make sense and align with dplyr::filter()'s logic. I also think that enabling NA-handling as coded again would be backwards compatible, since any existing code must be coercing (or otherwise explicitly-handling) NAs anyhow.

FWIW, the documentation doesn't explicitly mention NA-handling, but states:

  • for keep: "all elements where .p evaluates to TRUE", which would be valid as coded
  • for discard: "all elements where .p evaluates to FALSE", which is not exactly valid as coded, as discard()'s definition also retains NAs (if where_if() didn't throw).

Alternatively, adding something like .na = c(TRUE, FALSE) parameter (with match.arg) would be a clear & compact way to make such choices explicit, while removing common coercion cruft in .p.

Cheers!

{purrr} v1.0.2

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

No branches or pull requests

1 participant