last/first get argument na.rm

[ben-schwen]

Towards #4446
Towards #4239

Split into two different PRs now. This one introduces gforce_dynamic and make GForce work for first/last with different n

[ben-schwen]

edit: consistency issue fixed

options(datatable.verbose=TRUE)
DT = data.table(a=c(1:3,NA), b=1:2, c=c(1L, rep(NA, 3)))
options(datatable.optimize=2L)
DT[, last(.SD, na.rm=TRUE), b]
#> Argument 'by' after substitute: b
#> Finding groups using forderv ... forder.c received 4 rows and 1 columns
#> 0.000s elapsed (0.001s cpu) 
#> Finding group sizes from the positions (can be avoided to save RAM) ... 0.000s elapsed (0.000s cpu) 
#> Getting back original order ... forder.c received a vector type 'integer' length 2
#> 0.000s elapsed (0.000s cpu) 
#> lapply optimization changed j from 'last(.SD, na.rm = TRUE)' to 'list(last(a, na.rm = TRUE), last(c, na.rm = TRUE))'
#> GForce optimized j to 'list(glast(a, na.rm = TRUE), glast(c, na.rm = TRUE))'
#> Making each group and running j (GForce TRUE) ... gforce initial population of grp took 0.000
#> gforce assign high and low took 0.000
#> gforce eval took 0.000
#> 0.001s elapsed (0.001s cpu)
#>    b a  c
#> 1: 1 3  1
#> 2: 2 2 NA
options(datatable.optimize=1L)
DT[, last(.SD, na.rm=TRUE), b]
#> Argument 'by' after substitute: b
#> Finding groups using forderv ... forder.c received 4 rows and 1 columns
#> 0.000s elapsed (0.000s cpu) 
#> Finding group sizes from the positions (can be avoided to save RAM) ... 0.000s elapsed (0.000s cpu) 
#> Getting back original order ... forder.c received a vector type 'integer' length 2
#> 0.000s elapsed (0.001s cpu) 
#> lapply optimization changed j from 'last(.SD, na.rm = TRUE)' to 'list(last(a, na.rm = TRUE), last(c, na.rm = TRUE))'
#> Old mean optimization is on, left j unchanged.
#> Making each group and running j (GForce FALSE) ... last: using last(x[!is.na(x)]): na.rm=TRUE
#> last: using last(x[!is.na(x)]): na.rm=TRUE
#> last: using last(x[!is.na(x)]): na.rm=TRUE
#> last: using last(x[!is.na(x)]): na.rm=TRUE
#> 
#>   collecting discontiguous groups took 0.000s for 2 groups
#>   eval(j) took 0.000s for 2 calls
#> 0.000s elapsed (0.000s cpu)
#>    b a  c
#> 1: 1 3  1
#> 2: 2 2 NA
options(datatable.optimize=0L)
DT[, last(.SD, na.rm=TRUE), b]
#> Argument 'by' after substitute: b
#> Finding groups using forderv ... forder.c received 4 rows and 1 columns
#> 0.000s elapsed (0.001s cpu) 
#> Finding group sizes from the positions (can be avoided to save RAM) ... 0.000s elapsed (0.000s cpu) 
#> Getting back original order ... forder.c received a vector type 'integer' length 2
#> 0.000s elapsed (0.000s cpu) 
#> All optimizations are turned off
#> Making each group and running j (GForce FALSE) ... last: using x[, lapply(.SD, last, na.rm=TRUE)]): na.rm=TRUE
#> last: using last(x[!is.na(x)]): na.rm=TRUE
#> last: using last(x[!is.na(x)]): na.rm=TRUE
#> The result of j is a named list. It's very inefficient to create the same names over and over again for each group. When j=list(...), any names are detected, removed and put back after grouping has completed, for efficiency. Using j=transform(), for example, prevents that speedup (consider changing to :=). This message may be upgraded to warning in future.
#> last: using x[, lapply(.SD, last, na.rm=TRUE)]): na.rm=TRUE
#> last: using last(x[!is.na(x)]): na.rm=TRUE
#> last: using last(x[!is.na(x)]): na.rm=TRUE
#> 
#>   collecting discontiguous groups took 0.000s for 2 groups
#>   eval(j) took 0.001s for 2 calls
#> 0.001s elapsed (0.001s cpu)
#>    b a  c
#> 1: 1 3  1
#> 2: 2 2 NA

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.02%. Comparing base (e26cf1b) to head (b797eff).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5168   +/-   ##
=======================================
  Coverage   99.02%   99.02%           
=======================================
  Files          88       88           
  Lines       17376    17409   +33     
=======================================
+ Hits        17206    17239   +33     
  Misses        170      170           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ben-schwen]

@mattdowle I fixed expected behavior by turning off apply optimization for first/last. I also clarified in man that first/last always select at least 1 element/row. We might also add an "if present" to the former to exclude the special cases about empty vectors and empty data.tables.

[mattdowle]

iiuc, the || part here needs parens around it. and if so, there's a test missing because DT[, first(.SD, na.rm=TRUE), by=] is still being optimized but that's not intended. Will do ...
In fact I'm about to change it so that first(DT, na.rm=TRUE) does it column-wise, so then first(.SD, na.rm=TRUE) can be optimized with a consistent result. na.rm="row" can remove rows containing any NA.

[mattdowle]

Ok @ben-schwen and @MichaelChirico I'm finished with this one, please review. (Ben as you're the author GH wouldn't let me request a review from you in the top right.)

[MichaelChirico]

Thanks for the heavy lifting here Matt!! Mostly small comments.

[ben-schwen]

Should mention the padding with NA.

[mattdowle]

I'm a bit uncomfortable about the padding. The reason for it was because we don't have a way for GForce to tell the [.data.table caller how many items are in each group where that differs from min(n,grpsize) due to how many non-NA there are. If it wasn't for that then we'd return the first/last n non-NA as naturally expected I assume we all agree? If so then it's just our internals that are driving the user interface on this padding currently, which isn't usually a good reason to choose a behavior.
The trouble is, if we go ahead with the padding, then we'll have to keep that in future for backwards compatibility, and we'll have to add a controlling argument to provide the non-padding if and when implemented. I can't imagine anyone really wants the padding. After all, the intent with na.rm= is to remove NA not get more NA in some cases.
Shall I see how hard it might be to add that vector of the result size for each group to be returned from GForce? Might be useful in other functions too. Frustrating to still not finish this PR but now would be the time to get this right.

[ben-schwen]

True. Regarding speed we would have to iterate over the whole set twice? However, we would also be able to remove groups which do not contain non NA values which would quite useful, I guess.

What if we allocate enough space for min(n, grpsize) and then make a call to realloc?

[mattdowle]

Yes gfirstlast allocates space for min(n,grpsize) already and it could be resized down but it's more the ans = c(g, ans) afterwards in [.data.table (where g = lapply(g, rep.int, times=grplens)) that needs to know how many items each group has (where some groups have less than min(n,grpsize) non-NA). Also the eval(jsub, env) in gsumm.c:gforce would no longer create same-length or group-aligned vectors in the case of DT[, last(.SD, na.rm=TRUE, n=2), by=grp] where some groups might have one column with 1 non-NA and another with 2 non-NA for example. In that case, either gforce or [.data.table would need to NA pad the column containing the 1 non-NA in that group with an NA to align it with the other column that has 2 non-NA for that group. The result would be intuitive to the user as they would see no rows with all-NA unlike as this PR currently stands.
gfirstlast could attach an attribute to its result telling gforce how many in each group it found. If na.rm=FALSE it needn't bother allocating that attribute. Even when allocated it would only be ngrp long. Then if it didn't find any groups with less than min(n,grpsize) NA, it could remove the attribute so the caller knows that no padding needs to be done.
Maybe moving the g=lapply(g, rep.int, times=grplen); ans = c(g, ans) from [.data.table into gsumm.c:gforce will be necessary or just be tidier to have that in one place at the same time.

[ben-schwen]

Moving more group information into gsumm.c:gforce would not only benefit gfirstlast but is probably helpful/necessary for other future gforce functions such as gunique. (At least I had to do it on my last try on gunique which tried to resolve gunique with a single sort and kept track of whether elements are increasing per group).

[mattdowle]

Now done and called it gforce_dynamic. Currently gfirstlast and gshift attach this attribute (all other gfuns return one value per group). gsumm.c:gforce then works out the union shape of the potentially many gforce dynamic columns, aligns them, and reps the grpcols appropriately. Feels right now that the gfuns return their shape rather than having to code knowledge of the gfuns at R level.

[MichaelChirico]

If it's only output to test differently we can do c(1 = "output to match under optimize=1") (or reverse). adding to issue thread...

[jangorecki]

"wip shift multiple n return data.table rather than list" commit.
@mattdowle it looks like a serious breaking change. Was that even requested anywhere? Current approach worked well, and also has smaller overhead. If one uses shift output as a list (even: cols := shift(...)), then it is actually better to keep it as a list rather than data.table, at least by default.

[mattdowle]

@jangorecki shift multiple n returning list rather than data.table was causing problems in this PR yes. Since a data.table is a list too, I don't see much of an issue. But as I wrote it is wip. Since the list returned contains vectors of the same length, marking it as a data.table so anything using it knows it has a regular shape makes some sense to me. I don't think calling it out as a 'serious' breaking change helps.

[MichaelChirico]

@ben-schwen do you want to update this PR to master to proceed with review for 1.16.0?

[MichaelChirico]

Ping @ben-schwen appreciate your help resolving conflicts here :)

[ben-schwen]

Ping @ben-schwen appreciate your help resolving conflicts here :)

Will do. Not sure if I find the time today but definitely over the next week!

[MichaelChirico]

Got through part of the conflict resolution but ran out of time. committed with conflict markers still in place for now.

Done all but gsumm.c. The changes there are pretty substantial and hard to untangle. Maybe a rebase would be easier to do (untangling the changes one commit at a time), but will definitely consume a ton of time. I'm pulling this off the 1.16.0 milestone for now, but feel free to take it up & ping for review.

[HughParsonage]

SET_TRUELENGTH is not API

[ben-schwen]

Got through part of the conflict resolution but ran out of time. committed with conflict markers still in place for now.

Done all but gsumm.c. The changes there are pretty substantial and hard to untangle. Maybe a rebase would be easier to do (untangling the changes one commit at a time), but will definitely consume a ton of time. I'm pulling this off the 1.16.0 milestone for now, but feel free to take it up & ping for review.

That was my plan. Matt's change in gsumm.c look favorable and make sense but I want to split them into multiple PRs since they are hard to grasp

[MichaelChirico]

@ben-schwen I'm wondering whether to include this for 1.17.0, WDYT?

[ben-schwen]

@ben-schwen I'm wondering whether to include this for 1.17.0, WDYT?

These changes almost certainly have a stumbling block. I'm already working on the PR for it but wouldn't pull it into the 1.17 release. I guess we better aim for a stable release.