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

implement {map,map2,pmap}_{lgl,int,dbl,chr,raw}_matrix() #801

Closed
wants to merge 1 commit into from

Conversation

AshesITR
Copy link
Contributor

@AshesITR AshesITR commented Nov 3, 2020

closes #798

I'd like to have general feedback on the approach before creating tests and documentation.
The PR adds a new family of mapping functions, ending in _matrix. They have a new required argument, .n, the number of elements that .f returns in a single call and an optional argument, .by_row = FALSE that allows transposing the result matrix.

Default behaviour returns a matrix with .n rows and length(.x) columns of the respective type.

  • Are the argument names and locations fine?
  • The C implementation has a lot of duplication from map_impl, map2_impl and pmap_impl. Would it be worth deduplicating the codebase by altering / extending the original _impl functions?
  • implement
  • test
  • document

@AshesITR
Copy link
Contributor Author

AshesITR commented Nov 3, 2020

Ubuntu on 3.2 fails with

Error in loadNamespace(j <- i[[1L]], c(lib.loc, .libPaths()), versionCheck = vI[[j]]) :
there is no package called ‘digest’

digest is an Import of {rcmdcheck}.

{remotes} says digest is not available in
https://github.com/tidyverse/purrr/pull/801/checks?check_run_id=1349703165#step:8:17

@lionel-
Copy link
Member

lionel- commented Nov 4, 2020

Hello,

Thank you for looking into this. However it's probably a bad time for such a contribution. We will change the internals of purrr to vctrs in the next major version. At that point you could supply a ptype to get a matrix output. I think that would be preferable to extending the API of purrr for seldomly used target types.

@AshesITR
Copy link
Contributor Author

AshesITR commented Nov 4, 2020

Hello lionel,

thanks for your feedback. Are there issues I can help work on for this step?
I'd really love to have fast map_*_matrix functions available as I see many uses for them in numerical applications.

@lionel-
Copy link
Member

lionel- commented Nov 4, 2020

You can try wrapping around r-lib/vctrs#1227 and see if you can implement map_lgl_matrix() etc. If that is sufficient, these functions could eventually go in a separate package rather than in purrr.

There probably are performance issues in vctrs with matrices and arrays. If that's the case, reports would be helpful.

FYI I'm likely not resuming work on vctrs until December or January.

@hadley
Copy link
Member

hadley commented Aug 24, 2022

Thanks for working on this, but per my comment in the related issue, at this time we don't want to expand the API of purrr this much. Either the upcoming vctrs work will help resolve the problem, or you might consider putting these functions in their own package.

@hadley hadley closed this Aug 24, 2022
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.

Add map_*_matrix functions similar to vapply with FUN.VALUE of length > 1.
3 participants