Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7874 +/- ##
=======================================
Coverage 99.01% 99.01%
=======================================
Files 88 88
Lines 17292 17297 +5
=======================================
+ Hits 17122 17127 +5
Misses 170 170 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Generated via commit e66c968 Download link for the artifact containing the test results: ↓ atime-results.zip
|
mcol
reviewed
Aug 1, 2026
| test(2382.10, subset(DT, a > 5, drop=TRUE), subset(DF, a > 5, drop=TRUE)) | ||
|
|
||
| # #5489 frank could support reverse ranking | ||
| test(2383.01, frank(-as.Date(c("1992-02-27","1992-02-27","1992-01-14","1992-02-28","1992-02-01")), ties.method="min"), frankv(as.Date(c("1992-02-27","1992-02-27","1992-01-14","1992-02-28","1992-02-01")), order=-1, ties.method="min")) |
Contributor
There was a problem hiding this comment.
Writing tests like is seems clearer and more maintainable:
Suggested change
| test(2383.01, frank(-as.Date(c("1992-02-27","1992-02-27","1992-01-14","1992-02-28","1992-02-01")), ties.method="min"), frankv(as.Date(c("1992-02-27","1992-02-27","1992-01-14","1992-02-28","1992-02-01")), order=-1, ties.method="min")) | |
| x = as.Date(c("1992-02-27","1992-02-27","1992-01-14","1992-02-28","1992-02-01")) | |
| test(2383.01, frank(-x, ties.method="min"), frankv(x, order=-1, ties.method="min")) |
|
|
||
| 10. `subset()` method for data.tables supports `drop = TRUE` for consistency to data.frame, [#7859](https://github.com/Rdatatable/data.table/issues/7859). Thanks @MichaelChirico for the report and fix. | ||
|
|
||
| 11. `frank()` gains an `order` argument (matching `frankv()`) and now intercepts the unary minus symbol (e.g., `frank(-dates)`) to support reverse ranking even for types where unary `-` is not defined in R, such as `Date` or `character` vectors, [#5489](https://github.com/Rdatatable/data.table/issues/5489). Thanks @hope-data-science for the request and @venom1204 for teh implementation. |
Contributor
There was a problem hiding this comment.
Suggested change
| 11. `frank()` gains an `order` argument (matching `frankv()`) and now intercepts the unary minus symbol (e.g., `frank(-dates)`) to support reverse ranking even for types where unary `-` is not defined in R, such as `Date` or `character` vectors, [#5489](https://github.com/Rdatatable/data.table/issues/5489). Thanks @hope-data-science for the request and @venom1204 for teh implementation. | |
| 11. `frank()` gains an `order` argument (matching `frankv()`) and now intercepts the unary minus symbol (e.g., `frank(-dates)`) to support reverse ranking even for types where unary `-` is not defined in R, such as `Date` or `character` vectors, [#5489](https://github.com/Rdatatable/data.table/issues/5489). Thanks @hope-data-science for the request and @venom1204 for the implementation. |
ben-schwen
reviewed
Aug 3, 2026
| test(2383.03, frank(-idates), frankv(idates, order=-1L)) | ||
| it = as.ITime(c("10:00:00", "10:00:00", "08:00:00", "12:00:00", "09:00:00")) | ||
| test(2383.05, frank(-it), frankv(it, order=-1L)) | ||
| test(2383.04, frank(DT, -x, y), frankv(DT, cols=c("x","y"), order=c(-1L, 1L))) |
Member
There was a problem hiding this comment.
test order is wrong. and you probably would want to instantiate DT before using it and not use a DT from a previous test
ben-schwen
reviewed
Aug 3, 2026
|
|
||
| frank = function(x, ..., na.last=TRUE, ties.method=c("average", "first", "last", "random", "max", "min", "dense")) { | ||
| frank = function(x, ..., order=1L, na.last=TRUE, ties.method=c("average", "first", "last", "random", "max", "min", "dense")) { | ||
| if (missing(order)) { |
Member
There was a problem hiding this comment.
why are we only intercepting when order is missing, e.g. frank(-dates, order=-1L)
Shouldn't we warn the user in such cases?
ben-schwen
reviewed
Aug 3, 2026
| test(2383.05, frank(-it), frankv(it, order=-1L)) | ||
| test(2383.04, frank(DT, -x, y), frankv(DT, cols=c("x","y"), order=c(-1L, 1L))) | ||
| DT = data.table(a=c(1,2,1), b=c(1,1,2)) | ||
| test(2383.06, frank(DT, order=-1L), frankv(DT, order=-1L)) |
Member
There was a problem hiding this comment.
how should explicit order interact with -, e.g. frank(-x, order=-1L)
ben-schwen
reviewed
Aug 3, 2026
| dates = as.Date("2022-10-19") + c(1, 3, 2, 4) | ||
| frank(dates) # ascending | ||
| frank(-dates) # descending (intercepts '-' to avoid error) | ||
| frank(dates, order=-1L) # same as above |
Member
There was a problem hiding this comment.
Suggested change
| frank(dates, order=-1L) # same as above | |
| frank(dates, order=-1L) # also descending |
ben-schwen
reviewed
Aug 3, 2026
|
|
||
| dates = as.Date("2022-10-19") + c(1, 3, 2, 4) | ||
| frank(dates) # ascending | ||
| frank(-dates) # descending (intercepts '-' to avoid error) |
Member
There was a problem hiding this comment.
if we intercept - should we also intercept + for ascending order?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
closes #5489
this pr adds the order parameter to
frank()to bring it into parity withfrankv().Implements internal interception of the unary minus symbol in
frank(x). This allows reverse ranking for types where R does not define unary .