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

Deprecate the use of purrr #381

Closed
dajmcdon opened this issue Nov 27, 2023 · 6 comments
Closed

Deprecate the use of purrr #381

dajmcdon opened this issue Nov 27, 2023 · 6 comments

Comments

@dajmcdon
Copy link
Contributor

The {purrr} package is pretty beefy. It's better to use some light wrappers. See epipredict::compat-purrr or rlang::standalone-purrr.

@brookslogan
Copy link
Contributor

brookslogan commented Dec 8, 2023

Beefy in terms of install size, RAM, computational overhead? (I'm assuming rlang thing is for a different reason... they wanted purrr-style coding in a dependency of purrr [or, originally, maybe this was like v0 of purrr?].)

(Check on install size... in my R library it's 2MB vs. dplyr at 5MB, (broom at 5MB??,) rlang at ~6MB, readr at 9MB... [ + purrr comes packed with tidyverse so it's probably installed anyway])

@dajmcdon
Copy link
Contributor Author

dajmcdon commented Dec 8, 2023

Install but also speed. I think one of @nmdefries changes in that PR is actually along these lines. Basically just copy that linked file from rlang, so you can use purrr-style functions when needed without the overhead.

@dshemetov
Copy link
Contributor

Do we still want to do this? Is there more context for why we want to avoid purrr? I've never noticed it being a problem. @dajmcdon

@dshemetov dshemetov self-assigned this Jan 31, 2024
@dajmcdon
Copy link
Contributor Author

I try to avoid Imports/Dependencies where possible, especially for functions that are not user-facing. Even {dplyr} avoids the {purrr} import. I couldn't quickly find an explicit reason. But my guess is to avoid functional overhead for internal functions. Consider map_dbl(). The standalone is:

map_dbl <- function(.x, .f, ...) {
  .rlang_purrr_map_mold(.x, .f, double(1), ...)
}
.rlang_purrr_map_mold <- function(.x, .f, .mold, ...) {
  .f <- as_function(.f, env = global_env())
  out <- vapply(.x, .f, .mold, ..., USE.NAMES = FALSE)
  names(out) <- names(.x)
  out
}

while the {purrr} implementation needs a bunch of stuff. So "speed" is the likely reason.

https://github.com/r-lib/rlang/blob/main/R/standalone-purrr.R
https://github.com/tidyverse/dplyr/blob/main/R/import-standalone-purrr.R

@dshemetov
Copy link
Contributor

Without metrics this seems like a premature optimization to me. But it's also a very reversable decision, so I'm fine with doing it if you feel confident about it.

@dshemetov dshemetov removed their assignment Apr 11, 2024
@dshemetov
Copy link
Contributor

Gonna close this for reasons described in the OP in cmu-delphi/epipredict#395. We can revisit later, but definitely low prio right now.

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

3 participants