shift better naming for negative n

[jangorecki]

as discussed in inline comments to #3166
now when shift accepts negative integers we can get column names like V1_lag_-1 / V1_lead_-1, they look awkward.

• My suggestion is to use V1_shift_1 / V1_shift_-1 instead. This cannot be made without breaking change to current behavior, unless we introduce new type, just type="shift". Not much changes to code, and users who prefer to use only new interface (positive to lag, negative for lead) could switch to type="shift". Good to have it for 1.12.0 as we are releasing negative n feature there.

• Additionally on docker tests we are getting - fixed in 76d4a62 - issue was that match.arg was using shell dependent quoting

Test 1963.8 didn't produce the correct error :
Expected: should be one of "lag" 
Observed: 'arg' should be one of “lag”, “lead” 

those are environments striped out from many R stuff that is by default active, like methods package.

test(1963.8, shift(DT$x, type = 'some_other_type'),
     error = 'should be one of "lag"')

• fix invalid test 1963.4,, atomic vectors should not be named

[st-pasha]

The problem with a name such as V1_lag_-1 is that it cannot be used as a variable: DT[:, V1_lag_-1] will not work. Backticks may be used as a workaround, but they may not work everywhere, may not be widely known, and are more cumbersome.

What can be done is to automatically replace lag <=> lead upon getting a negative n. This way the produced names will be V1_lag_1 and V1_lead_1 respectively.

[MichaelChirico]

I agree w pasha here. the awkward part of the naming is exactly the -1 part, so proposed solution continues to be awkward IMO

…

On Mon, Dec 17, 2018, 7:05 PM Pasha Stetsenko ***@***.*** wrote: The problem with a name such as V1_lag_-1 is that it cannot be used as a variable: DT[:, V1_lag_-1] will not work. Backticks may be used as a workaround, but they may not work everywhere, may not be widely known, and are more cumbersome. What can be done is to automatically replace lag <=> lead upon getting a negative n. This way the produced names will be V1_lag_1 and V1_lead_1 respectively. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3223 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHQQdSeuoDDttzCiviFR91d5tclwpOp6ks5u6BUhgaJpZM4ZUknP> .

[jangorecki]

Backticks is not a workaround but standard way to deal with column names like this. From the programmatic point of view it is much better to deal with backticks than condition on your input to shift() to operate its results later on. Consistency is no less important than user friendliness.

[mattdowle]

Everyone can be satisfied here. In 32eee2b I kept new type="shift" and flipped the lead/lag in the names too. That way users can use whichever they prefer.
NEWS item now reads :

shift() now interprets negative values of n to mean the opposite type=, #1708. When give.names=TRUE the result is named using a positive n with the appropriate type. Alternatively, a new type="shift" names the result using a signed n and constant type.
shift(x, n=-5:5, give.names=TRUE) => "_lead_5" ... "_lag_5"
shift(x, n=-5:5, type="shift", give.names=TRUE) => "_shift_-5" ... "_shift_5"