-
Notifications
You must be signed in to change notification settings - Fork 1k
implement frev as fast base::rev alternative #5907
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
Changes from 33 commits
d8db6e3
c468589
95208ac
fe474d7
ed845d3
8ba2d2d
30ae580
9839ef5
3b6fa52
1d1b0df
320678d
6943ebd
67bd0c9
812a854
529028a
73d2fdb
b9e167c
59b59ab
88d1ff9
f85922a
e4324cf
18a7209
b6bd964
68f0e41
7e1a950
d9d17a7
da24f85
58608a2
c84a123
513f20f
daee139
f5ef168
f658ff4
53149ed
a99d32f
a56b796
1bef92c
6d6d1cd
a6907ad
1e9f481
07fbea8
a285661
4318bb7
86d3d59
c507fa5
08b3591
97ea3ff
df4f160
461a97a
025a3c5
48ded0b
526a4ed
be50528
f15ae3c
976d3ba
796828d
181957e
c751124
d02df36
2001816
3cc839c
e27a6f3
276cdeb
a7de0f8
300ea93
17319c6
b4fe534
832324c
7d6aea9
ccd9ee6
6b6da26
b2cde13
cabddd2
e0c2e48
bbbff7e
c646d54
bebc49e
be61f12
e9f30e5
d71c515
d2dc90f
7145955
5a2e73a
1395ee2
1a56a2c
7e44b6b
929f111
9fb00fe
ddc4ae9
a50dda9
0a6d09d
1e8c28a
c651d91
20fc7b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -202,3 +202,4 @@ S3method(format_list_item, default) | |
|
|
||
| export(fdroplevels) | ||
| S3method(droplevels, data.table) | ||
| export(frev) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18260,3 +18260,53 @@ test(2243.54, dt[, .I[j], x]$V1, c(1L, 3L), output="GForce TRUE") | |
| test(2243.55, dt[, .I[i], x]$V1, 1:4, output="GForce FALSE") | ||
| test(2243.56, dt[, .I[1:2], x]$V1, 1:4, output="GForce FALSE") | ||
| options(old) | ||
|
|
||
| # 5885 implement frev | ||
| d = c(NA, NaN, Inf, -Inf) | ||
| test(2244.00, frev(c(FALSE, NA), copy=TRUE), c(NA, FALSE)) | ||
| test(2244.01, frev(c(0L, NA), copy=TRUE), c(NA, 0L)) | ||
| test(2244.02, frev(1:3, copy=TRUE), 3:1) | ||
| test(2244.03, frev(d, copy=TRUE), c(-Inf, Inf, NaN, NA)) | ||
| test(2244.04, frev(c(NA, 1, 0+2i), copy=TRUE), c(0+2i, 1, NA)) | ||
| test(2244.05, frev(as.raw(0:1), copy=TRUE), as.raw(1:0)) | ||
| test(2244.06, frev(NULL, copy=TRUE), NULL) | ||
| test(2244.07, frev(character(5), copy=TRUE), character(5)) | ||
| test(2244.08, frev(integer(0), copy=TRUE), integer(0)) | ||
| test(2244.09, frev(list(1, "a"), copy=TRUE), list("a", 1)) | ||
| test(2244.11, frev(c(0L, NA), copy=FALSE), c(NA, 0L)) | ||
| test(2244.12, frev(1:3, copy=FALSE), 3:1) | ||
| test(2244.13, frev(d, copy=FALSE), c(-Inf, Inf, NaN, NA)) | ||
| test(2244.14, frev(c(NA, 1, 0+2i), copy=FALSE), c(0+2i, 1, NA)) | ||
| test(2244.15, frev(as.raw(0:1), copy=FALSE), as.raw(1:0)) | ||
| test(2244.16, frev(NULL, copy=FALSE), NULL) | ||
| test(2244.17, frev(character(5), copy=FALSE), character(5)) | ||
| test(2244.18, frev(integer(0), copy=FALSE), integer(0)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Recommend adding a test for ALTREP vectors (e.g.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests needs to be light, therefore I would comment out those tests after running and confirming they work as expected.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For making if (ALTREP(x) && R_altrep_data2(x) == R_NilValue) {
SEXP info = R_altrep_data1(x);
R_xlen_t LENGTH = (R_xlen_t)REAL0(info)[0];
int FIRST = (int)REAL0(info)[1];
int INCR = (int)REAL0(info)[2];
REAL0(info)[1] = FIRST+(LENGTH-1)*INCR;
REAL0(info)[2] = INCR*(-1);
R_set_altrep_data1(x, info);
UNPROTECT(nprotect);
return x;
}
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will eat a lot of memory. Matt spent quite some time making tests memory conservative. Any bigger tests should go into another script not run by default. Or be commented out, and confirmed it worked at the time or PR.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Altrep is easiest done at the R level (see below), but if there's currently no interface (i.e. no frev <- function(x) {
if (is_altrep(x)) {
return(last(x):first(x))
}
....
}
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @HughParsonage Thats actually such a big brain move. Ty for that!
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Till now we always unpacked altrep in DT. |
||
| test(2244.19, frev(list(1, "a"), copy=FALSE), list("a", 1)) | ||
| test(2244.20, frev(1:1e2, copy=TRUE), rev(1:1e2)) | ||
| # copy arguments | ||
| x = 1:3 | ||
| test(2244.21, {frev(x, copy=TRUE); x}, 1:3) | ||
| test(2244.22, {frev(x, copy=FALSE); x}, 3:1) | ||
| # levels | ||
| f = as.factor(letters) | ||
| test(2244.31, frev(f, copy=TRUE), rev(f)) | ||
| test(2244.32, frev(as.IDate(1:10), copy=TRUE), as.IDate(10:1)) | ||
| test(2244.33, frev(as.IDate(1:10), copy=TRUE), as.IDate(10:1)) | ||
| # names | ||
| x = c(a=1L, b=2L, c=3L) | ||
| test(2244.41, frev(x, copy=TRUE), rev(x)) | ||
| test(2244.42, frev(x, copy=FALSE), x) | ||
| # attributes | ||
| x = structure(1:10, class = c("IDate", "Date"), att = 1L) | ||
| test(2244.51, attr(frev(x, copy=TRUE), "att"), 1L) | ||
| test(2244.52, attr(frev(x, copy=FALSE), "att"), 1L) | ||
| # errors | ||
| test(2244.61, frev(data.table()), error="should not be data.frame or data.table") | ||
| test(2244.62, frev(1:2, copy=NA), error="must be TRUE or FALSE") | ||
| test(2244.63, frev(expression(1)), error="is not supported by frev") | ||
| test(2244.64, frev(matrix(1)), error="should not be matrix or array") | ||
| if (test_bit64) { | ||
| x = as.integer64(c(1, NA, 3)) | ||
| test(2244.71, frev(x, copy=TRUE), rev(x)) | ||
| test(2244.72, frev(x, copy=FALSE), x) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| \name{frev} | ||
| \alias{frev} | ||
| \alias{rev} | ||
| \title{Fast reverse} | ||
| \description{ | ||
| Similar to \code{base::rev} but \emph{much faster}. | ||
|
ben-schwen marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| \usage{ | ||
| frev(x, copy=TRUE) | ||
| } | ||
| \arguments{ | ||
| \item{x}{ An atomic \code{vector} or \code{list}. } | ||
| \item{copy}{ logical (default is \code{TRUE}). If \code{FALSE} reversing happens in-place. } | ||
| } | ||
|
|
||
| \value{ | ||
| \code{frev} returns the input reversed. | ||
| } | ||
|
|
||
| \examples{ | ||
| # on vectors | ||
| x = setNames(1:26, letters) | ||
| frev(x[1:10]) | ||
| # reverse in-place | ||
| frev(x, copy=FALSE) | ||
| x | ||
|
|
||
| # list | ||
| frev(list(1, "a")) | ||
| } | ||
| \keyword{ data } | ||
Uh oh!
There was an error while loading. Please reload this page.