Conversation
Codecov Report
@@ Coverage Diff @@
## master #4338 +/- ##
=======================================
Coverage 99.42% 99.42%
=======================================
Files 73 73
Lines 14455 14459 +4
=======================================
+ Hits 14372 14376 +4
Misses 83 83
Continue to review full report at Codecov.
|
|
Not 100% convinced of the need for it, but it's clearly come up for you a few times over the years, so no reason to keep an error in a demonstrated use case. Changes LGTM |
|
If a user want to have a flexibility in parameterizing sorted or unsorted results, then duplicating whole data.table query and branching on top of it was needed. With this PR it is a matter of passing a flag into single query. if (!sorted) {
DT[, .SD, by = grp]
} else {
DT[, SD, keyby = grp]
}vs DT[, .SD, by = grp, keyby = sorted] |
|
One example is that I want to compare benchmark script of current queries ( |
mattdowle
changed the title
mattdowle
changed the title
…st to convey a bit more to the reader of the test
| \emph{Advanced:} In the \code{X[Y, j]} form of grouping, the \code{j} expression sees variables in \code{X} first, then \code{Y}. We call this \emph{join inherited scope}. If the variable is not in \code{X} or \code{Y} then the calling frame is searched, its calling frame, and so on in the usual way up to and including the global environment.} | ||
|
|
||
| \item{keyby}{ Same as \code{by}, but with an additional \code{setkey()} run on the \code{by} columns of the result, for convenience. It is common practice to use `keyby=` routinely when you wish the result to be sorted.} | ||
| \item{keyby}{ Logical scalar indicating if groups defined in \code{by} argument should be sorted. If \code{by} is missing, then \code{keyby} can also take the same input as \code{by} would, but results will be sorted.} |
There was a problem hiding this comment.
In addition to allowing keyby=TRUE/FALSE which is great, this PR seems to tilt the emphasis too: the default for keyby= in the [.data.table signature is now FALSE and the argument description here starts with 'Logical scalar ...'. Combined, these changes convey, to me at least, that we prefer keyby=TRUE/FALSE now.
- Can the default for
keybybe restored to be nothing, as it was before. That way, thekeyby=FALSEdoesn't convey that we preferTRUE/FALSE. I still much prefer seeingkeyby=<expression>, rather than bothby=andkeyby=. - Can this manual page item be changed back to how it was, with an extra sentence at the end saying that
keyby=can also be TRUE/FALSE too whenby=is provided.
…, and 2 substitute calls instead of 3. I was finding myself comparing the split-out cases before to see if everything had been remembered in each one.
3 participants
closes #4307