Support negative values of n in shift by MichaelChirico · Pull Request #3166 · Rdatatable/data.table

MichaelChirico · 2018-11-30T09:11:48Z

My first non-trivial (but also basically trivial) foray into C code so some careful eyes are warranted. I don't think I changed much but still..

Note well the two comments to this PR

…shift

MichaelChirico · 2018-11-30T09:12:54Z

-      default :
-        error("Unsupported type '%s'", type2char(TYPEOF(elem)));
-      }
-      copyMostAttrib(elem, tmp);


I honestly thing these lines are vestigial and I think they may be slowing down shift unnecessarily... there's no analogue in the 'lead' branch and I think what it's accomplishing is already done in the INTSXP branch.

which "these" precisely? what is it is accomplishing that is already done in INTSXP?

MichaelChirico · 2018-11-30T09:13:46Z

+     c(2L, 3L, 4L, 5L, 6L, 7L, 8L, 9L, 10L, 0L))
+test(1960.4, shift(DT$x, -1, give.names = TRUE),
+     structure(c(2L, 3L, 4L, 5L, 6L, 7L, 8L, 9L, 10L, NA),
+     .Names = c("V1_lag_-1", NA, NA, NA, NA, NA, NA, NA, NA, NA)))


This is certainly awkwardly named. The naming is done at the R level, so it would be very easy for me to change this to V1_lead_1. Any thoughts?

better than lag/lead in this case to use shift_1 or shift_-1

this test is invalid, we should not set name on vector result, only for list results it make sense, will fix it as part of #3223 - unrelated to shift vs lag/lead naming

codecov · 2018-11-30T09:30:10Z

Codecov Report

Merging #3166 into master will decrease coverage by 2.42%.
The diff coverage is 98.82%.

@@            Coverage Diff             @@
##           master    #3166      +/-   ##
==========================================
- Coverage    94.6%   92.17%   -2.43%     
==========================================
  Files          61       61              
  Lines       11747    11545     -202     
==========================================
- Hits        11113    10642     -471     
- Misses        634      903     +269

Impacted Files	Coverage Δ
src/shift.c	`97.47% <98.82%> (-0.04%)`	⬇️
R/uniqlist.R	`71.42% <0%> (-28.58%)`	⬇️
src/fsort.c	`74.09% <0%> (-21.72%)`	⬇️
src/between.c	`80% <0%> (-20%)`	⬇️
R/frank.R	`82.43% <0%> (-17.57%)`	⬇️
R/duplicated.R	`77.92% <0%> (-16.68%)`	⬇️
R/merge.R	`81.81% <0%> (-16.67%)`	⬇️
R/setkey.R	`83.33% <0%> (-15.37%)`	⬇️
R/setops.R	`84.92% <0%> (-14.04%)`	⬇️
R/foverlaps.R	`86.11% <0%> (-13.89%)`	⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02642ab...d9f1286. Read the comment docs.

MichaelChirico · 2018-11-30T10:59:43Z

Codecov doesn't appear to have re-triggered with the second commit; can do manually?

jangorecki

Just for future, if you see an issue that someone is assigned to, prompt that person about it before starting development. I started to refactor shift for that change, handling negative n without top level branching, also having bigger scope like removing DATAPTR calls, parallel processing for multiple columns/windows. My branch is far from being done and now my queue is long so we can proceed with your PR 👍 Nice to see you moving into more C coding!

jangorecki · 2018-11-30T11:07:27Z

+     c(2L, 3L, 4L, 5L, 6L, 7L, 8L, 9L, 10L, 0L))
+test(1960.4, shift(DT$x, -1, give.names = TRUE),
+     structure(c(2L, 3L, 4L, 5L, 6L, 7L, 8L, 9L, 10L, NA),
+     .Names = c("V1_lag_-1", NA, NA, NA, NA, NA, NA, NA, NA, NA)))


better than lag/lead in this case to use shift_1 or shift_-1

jangorecki · 2018-11-30T11:12:12Z

-      default :
-        error("Unsupported type '%s'", type2char(TYPEOF(elem)));
-      }
-      copyMostAttrib(elem, tmp);


which "these" precisely? what is it is accomplishing that is already done in INTSXP?

jangorecki · 2018-11-30T11:15:46Z

-  \item{type}{ default is \code{"lag"}. The other possible value is \code{"lead"}. }
+  \item{n}{ integer vector denoting the offset by which to lead or lag the input. To create multiple lead/lag vectors, provide multiple values to \code{n}; negative values of \code{n} will "flip" the value of \code{type}, i.e., \code{n=-1} and \code{type='lead'} is the same as \code{n=1} and \code{type='lag'}. }
+  \item{fill}{ Value to use for padding when the window goes beyond the input length. }
+  \item{type}{ default is \code{"lag"} (look "backwards"). The other possible value is \code{"lead"} (look "forwards"). }


I would more strongly suggest to use negative n instead of lead.

MichaelChirico · 2018-11-30T16:08:50Z

@jangorecki oh my! i didn't even bother to check TBH, my bad.

I knew this would have some overlap with your work on rolling operations in #2961, I should have read around a bit more carefully.

Agree there's room for more improvement w parallelism but I saw that the switch to allowing +/- n in the current code base would be an easy switch and went for it 🔪

jangorecki · 2018-11-30T16:26:46Z

easy when you do top if branching :) I think it is possible take out memmove from if branch, similarly as I did for align=center/left in

data.table/src/froll.c

Lines 24 to 26 in 70208d9

    
           int k_ = align==-1 ? k-1 : floor(k/2);                      // offset to shift 
        
           if (verbose) Rprintf("%s: align %d, shift answer by %d\n", __func__, align, -k_); 
        
           memmove((char *)ans, (char *)ans + (k_*sizeof(double)), (nx-k_)*sizeof(double)); // apply shift to achieve expected align

mattdowle

Agree that coverage isn't kicking in on this one: the coverage that is displayed above is very wrong/stale. I just merged in master and resolved conflicts which usually kicks off a refresh, but it either didn't or is taking a very long time. I'm not aware of any way to correct coverage manually. They seem to be having technical issues generally; e.g. we've been seeing multiple comments posted by the codecov bot too in other PRs.

So, taking a pragmatic approach ...

Code removal in shift.c very nice. It had two large switches through column types, but only differing in one doing lead and the other lag. You've folded it into one with a signed +/-n. Much better.
I checked coverage of master::shift.c which is at 97.52%. Only 4 lines are missed and they are all error(). So that tells us that tests cover every case in both the old lead and lag branch which is good.
No existing tests change and you've added new ones. So even though a lot of C code is removed, everything still works and that isn't because the removed code had no coverage. Merging this PR to master should update master coverage and if there's any problems it can always be fixed post merge.

Great!

Closes #1708 -- add support for postiive and negative values of n in …

578e953

…shift

MichaelChirico commented Nov 30, 2018

View reviewed changes

coverage tests

e0c05fa

MichaelChirico force-pushed the shift_negative_n branch from 7ddbd62 to e0c05fa Compare November 30, 2018 09:41

jangorecki reviewed Nov 30, 2018

View reviewed changes

mattdowle added 2 commits December 5, 2018 13:23

Merge branch 'master' into shift_negative_n

be30247

Merge branch 'master' into shift_negative_n

d9f1286

mattdowle added this to the 1.12.0 milestone Dec 14, 2018

mattdowle changed the title ~~Closes #1708 -- add support for positive and negative values of n in shift~~ Support negative values of n in shift Dec 14, 2018

mattdowle approved these changes Dec 14, 2018

View reviewed changes

mattdowle merged commit c434610 into master Dec 14, 2018

mattdowle deleted the shift_negative_n branch December 14, 2018 23:16

jangorecki mentioned this pull request Dec 15, 2018

shift better naming for negative n #3223

Closed

3 tasks

Conversation

MichaelChirico commented Nov 30, 2018 • edited by mattdowle Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelChirico Nov 30, 2018

Choose a reason for hiding this comment

Uh oh!

jangorecki Nov 30, 2018

Choose a reason for hiding this comment

Uh oh!

MichaelChirico Nov 30, 2018

Choose a reason for hiding this comment

Uh oh!

jangorecki Nov 30, 2018

Choose a reason for hiding this comment

Uh oh!

jangorecki Dec 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

MichaelChirico commented Nov 30, 2018

Uh oh!

jangorecki left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jangorecki Nov 30, 2018

Choose a reason for hiding this comment

Uh oh!

jangorecki Nov 30, 2018

Choose a reason for hiding this comment

Uh oh!

jangorecki Nov 30, 2018

Choose a reason for hiding this comment

Uh oh!

MichaelChirico commented Nov 30, 2018

Uh oh!

jangorecki commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattdowle left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MichaelChirico commented Nov 30, 2018 •

edited by mattdowle

Loading

jangorecki Dec 15, 2018 •

edited

Loading

codecov bot commented Nov 30, 2018 •

edited

Loading

jangorecki left a comment •

edited

Loading

jangorecki commented Nov 30, 2018 •

edited

Loading

mattdowle left a comment •

edited

Loading