keep argument for transpose and tstrsplit - #7883
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7883 +/- ##
==========================================
- Coverage 99.01% 99.00% -0.02%
==========================================
Files 88 88
Lines 17309 17323 +14
==========================================
+ Hits 17139 17151 +12
- Misses 170 172 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Generated via commit 14cbe56 Download link for the artifact containing the test results: ↓ atime-results.zip
|
joshhwuu
left a comment
joshhwuu
left a comment
There was a problem hiding this comment.
took a quick stab. C is not really my forte so I'll leave that up to someone else:)
the PR motivation cites performance benefits. is there some way we can measure this / regression test?
| \item{keep.names}{The name of the first column in the result containing the names of the input; e.g. \code{keep.names="rn"}. By default \code{NULL} and the names of the input are discarded.} | ||
| \item{make.names}{The name or number of a column in the input to use as names of the output; e.g. \code{make.names="rn"}. By default \code{NULL} and default names are given to the output columns.} | ||
| \item{list.cols}{Default is \code{FALSE}. \code{TRUE} will avoid promoting types and return columns of type \code{list} instead. \code{factor} will always be cast to \code{character}.} | ||
| \item{keep}{An integer vector of column indices to keep and return. The columns will be returned in the order specified. If \code{NULL} (default), all columns are returned. This is much more memory efficient than transposing the entire list and then subsetting the result.} |
There was a problem hiding this comment.
This is much more memory efficient than transposing the entire list and then subsetting the result.
not sure if its necessary to justify this use case here
| lapply(seq(length(l[[1]])), function(x) lapply(l, `[[`, x)) | ||
| transpose(l, list.cols=TRUE) | ||
|
|
||
| ll = list(nm=c('x', 'y'), 1:2, 3:4) |
There was a problem hiding this comment.
why do we remove these examples?
| @@ -1,4 +1,8 @@ | |||
| transpose = function(l, fill=NA, ignore.empty=FALSE, keep.names=NULL, make.names=NULL, list.cols=FALSE) { | |||
| transpose = function(l, fill=NA, ignore.empty=FALSE, keep.names=NULL, make.names=NULL, list.cols=FALSE, keep=NULL) { | |||
There was a problem hiding this comment.
i didnt look but could there be any regressions from folks using keep instead of keep.names explicitly?
|
|
||
| # transpose() supports keep argument, #5250 | ||
| x = list(1:5, 6:10, 11:15) | ||
| test(2388.01, transpose(list(1:5), keep="a"), error="'keep' must be an integer vector.") |
There was a problem hiding this comment.
are tests .01 and .10 not identical?
| transpose = function(l, fill=NA, ignore.empty=FALSE, keep.names=NULL, make.names=NULL, list.cols=FALSE, keep=NULL) { | ||
| if (!is.null(keep)) { | ||
| if (!is.numeric(keep)) stopf("'keep' must be an integer vector.") | ||
| keep = as.integer(keep) |
2 participants
closes #5250
This PR implements the
keepargument fortranspose()andtstrsplit()by moving selective column logic directly into the C engine. By only allocating memory for requested indices insrc/transpose.c, the implementation prevents the memory spikes and throwaway column allocations previously associated with subsetting large string-split results.