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

Give warning for early atomic conditions in case_when #6012

Closed
wants to merge 4 commits into from

Conversation

eutwt
Copy link
Contributor

@eutwt eutwt commented Sep 11, 2021

Warning to alert user of single-valued conditions when not all conditions are single-valued. Does not warn for last case (TRUE ~ RHS is okay for last case).

For example, conditions resulting from mixing && and & as in tidyverse/dtplyr#300. Maybe should be an error at some point, but that would of course be a breaking change for code depending on current behavior.

Current dplyr:

library(dplyr, warn.conflicts = FALSE)

df <- data.frame(a = 1:5, b = 11:15)

df %>% 
  mutate(grp = 
    case_when(
      a == 3 && b < 15 ~ 'grp 1',
      b > 15 ~ 'grp 2',
      TRUE ~ 'other'
    )
  )
#>   a  b   grp
#> 1 1 11 other
#> 2 2 12 other
#> 3 3 13 other
#> 4 4 14 other
#> 5 5 15 other

Created on 2021-09-11 by the reprex package (v2.0.1)

This PR:

library(dplyr, warn.conflicts = FALSE)

df <- data.frame(a = 1:5, b = 11:15)

df %>% 
  mutate(grp = 
    case_when(
      a == 3 && b < 15 ~ 'grp 1',
      b > 15 ~ 'grp 2',
      TRUE ~ 'other'
    )
  )
#> Warning: LHS of case 1 (`a == 3 && b < 15 ~ "grp 1"`) is a single value and will
#> be recycled.
#>   a  b   grp
#> 1 1 11 other
#> 2 2 12 other
#> 3 3 13 other
#> 4 4 14 other
#> 5 5 15 other

Created on 2021-09-11 by the reprex package (v2.0.1)

Actually in RStudio I get the following additional warning lines added by mutate, not sure why they don't show in reprex

Problem with `mutate()` column `a`.`a = case_when(1:32 == 1 ~ 1:32, 3 == 4 ~ 2L, TRUE ~ 3L)`.

@eutwt
Copy link
Contributor Author

eutwt commented Sep 11, 2021

Currently the warning only references the first single-valued condition. Not sure whether to say something like LHS of cases 2, 3, and 4 .... With a warning listing all offending cases, should any expressions be shown?

Also, is it commonly known what LHS means? Is "condition" more widely understood? I think most reference material for SQL case statements uses the word "condition", but abort_case_when_logical uses "LHS".

@romainfrancois
Copy link
Member

This looks good to me, however we will implement a newer case_when() in funs:: and perhaps this should be a feature request in tidyverse/funs#38

@eutwt
Copy link
Contributor Author

eutwt commented Oct 12, 2021

Closing in favor of moving to feature request in tidyverse/funs#38

@eutwt eutwt closed this Oct 12, 2021
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 this pull request may close these issues.

2 participants