Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 frev as fast base::rev alternative #5907
base: master
Are you sure you want to change the base?
implement frev as fast base::rev alternative #5907
Changes from 19 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
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommend adding a test for ALTREP vectors (e.g.
1:1e6
) and long vectors (e.g. 1:1e10).There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For making
1:1e10
work we would need to support ALTREP. Supporting ALTREP forcopy=FALSE
seems rather straightforward but creating new ALTREP objects seems to be a bit clunky.There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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
is_altrep(x)
in data.table already) I withdraw my recommendation to include it in the test suite here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HughParsonage Thats actually such a big brain move. Ty for that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Till now we always unpacked altrep in DT.