-
Notifications
You must be signed in to change notification settings - Fork 990
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
by= ignored if there is no j expression #3263
Comments
Related #1269 (comment) and later comments |
Not sure I follow your reasoning for your second example... is that for convenience? easy to imagine I have some complicated expression |
One could argue whether However, I don't think silently ignoring the |
definitely agreed, and in fact I think this is natural syntax for just
returning the groups. but in that case this issue is a duplicate of the
issue Frank referenced IIUC. the novelty here was your second example which
I don't quite follow
…On Thu, Jan 10, 2019, 8:28 AM Pasha Stetsenko ***@***.*** wrote:
One could argue whether DT[,,B] makes sense or not.
If it doesn't, then an error should be thrown.
If it does, then the result should be the DT reordered in such way that
the values in column B are grouped together. And we should also decide
whether B ought to be moved to the front or kept in its place, or both.
However, I don't think silently ignoring the by argument is the right
thing to do...
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#3263 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHQQdSw72is90-yCUv7J8_SPbfL5IQejks5vBolKgaJpZM4Z4JJD>
.
|
But the second example already does the right thing: |
Please use dev not release when reporting bugs. > DT = data.table(A=c(1,2,1,2,1,2), B=c(1,2,1,1,2,2))
> DT[, , by=A+B]
...
Warning message:
In `[.data.table`(DT, , , by = A + B) :
i and j are both missing so ignoring the other arguments
> |
@st-pasha sorry I see the confusion, I was referring to your comment:
|
Closing because the warning is there and that was the focus of this issue. |
The text was updated successfully, but these errors were encountered: