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

Optimize if_else/coalesce to fifelse/fcoalesce? #112

Closed
MichaelChirico opened this issue Oct 9, 2019 · 4 comments

Comments

@MichaelChirico
Copy link
Contributor

@MichaelChirico MichaelChirico commented Oct 9, 2019

data.table now has its own implementation of if_else (fifelse) and coalesce (fcoalesce).

I just tried the following: add two lines to dplyr/tests/testthat.R:

library(testthat)
library(dplyr)
if_else = data.table::fifelse
coalesce = data.table::fcoalesce

test_check("dplyr")

then run testthat::test_dir('tests').

7 tests failed, but the same tests failed if I removed those two lines, so I'm taking that to mean fifelse and fcoalesce pass usage tests dplyr for behavior users expect of those functions.

Does it make sense/would it be straightforward to optimize those functions when used in dtplyr?

@hadley

This comment has been minimized.

Copy link
Member

@hadley hadley commented Oct 10, 2019

Yeah, I think that makes sense — it's just a matter of adding a few more lines to https://github.com/tidyverse/dtplyr/blob/master/R/tidyeval.R#L61-L75

@hadley

This comment has been minimized.

Copy link
Member

@hadley hadley commented Nov 2, 2019

@MichaelChirico I'm likely to be working on this in the next week if you want to take a stab at it. (If you don't have time, don't worry, as I can also do it myself)

@MichaelChirico

This comment has been minimized.

Copy link
Contributor Author

@MichaelChirico MichaelChirico commented Nov 2, 2019

@MichaelChirico

This comment has been minimized.

Copy link
Contributor Author

@MichaelChirico MichaelChirico commented Nov 3, 2019

Heads up that we've also got fcase in the pipeline, though we opted for a when-value pair API as an initial pass, so it won't be a simple drop-in to the formula interface of case_when:

Rdatatable/data.table#4021

MichaelChirico added a commit to MichaelChirico/dtplyr that referenced this issue Nov 3, 2019
@hadley hadley closed this in 93d3790 Nov 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.