Wrap rolling sum updateBy operation

[jmao-denver]

Fixes #3388

1. added rolling_sum_tick() and rolling_sum_time()
2. added test_rolling_sum and test_rolling_proxy in test_updateby.py
3. some housekeeping in test_updateby and ugp.auto_locking (default true now) related code in tests, should make it easier to add test cases for new updateBy operations in the near future

[lbooker42]

LGTM, thanks!

[chipkent]

Seeing this made me wonder if we made an API mistake. For the rolling sum, ema, etc, we have separate methods for different flavors (e.g. ema_tick_decay and ema_time_decay). It seems like it might be more pythonic to have ema and rolling_sum, where the flavor is determined by the inputs. I'm not sure how strong my thoughts are, so this is more of a discussion item.
@jmao-denver @jjbrosnan

[jmao-denver]

I can see the benefits of folding them into one function, but I prefer to have separate functions (especially when the parameter sets are quite different), as it is more explicit, cleaner, less chance for users to make mistake as far as arguments are concerned, also easier for us to document/maintain the code.

[lbooker42]

I don't mind the explicitness of the ticks/time signatures but I have near-zero python experience in a professional role.

[jjbrosnan]

I'm not sure the parameter sets need to be that different. If these were combined into one function, you could have the first argument ts_col be optional with a default value of None. The rest of the arguments could look like they do in the time-based rolling sum functions, where each argument must be an int or string. In a tick case, you give ints. In the time case, if you give an int, it assumes seconds, and if you give a string, it can be whatever.

Then, the try/except block at the end can be separated into two parts, which depend on whether or not a timestamp column is given.

I think the benefits of combining each into one function outweigh the drawbacks.

[jjbrosnan]

Essentially, for the rolling sum, it would be (without comments):

def rolling_sum(ts_col: str = None, cols: Union[str, List[str]], rev_time: Union[int, str],
                     fwd_time: Union[int, str] = 0) -> UpdateByOperation:
    if ts_col:
        try:
            cols = to_sequence(cols)
            rev_time = _JDateTimeUtils.expressionToNanos(rev_time) if isinstance(rev_time, str) else rev_time
            fwd_time = _JDateTimeUtils.expressionToNanos(fwd_time) if isinstance(fwd_time, str) else fwd_time
            return UpdateByOperation(j_updateby_op=_JUpdateByOperation.RollingSum(ts_col, rev_time, fwd_time, *cols))
        except Exception as e:
            raise DHError(e, "failed to create a rolling sum (time) UpdateByOperation.") from e
    else:
        try:
            cols = to_sequence(cols)
            return UpdateByOperation(j_updateby_op=_JUpdateByOperation.RollingSum(rev_ticks, fwd_ticks, *cols))
        except Exception as e:
            raise DHError(e, "failed to create a rolling sum (tick) UpdateByOperation.") from e

[lbooker42]

@jjbrosnan some extra testing in the ticks case will be needed. Time strings need to be rejected as they make no sense for row-based computations. You will get an exception from the call, so as long as the error is clear this looks good.

[jmao-denver]

Yes, it is doable (we'll need to add checks for invalid argument combinations) but to me the extra cost and the potential cognitive burden on a user to understand the difference outweighs the gains.

[jmao-denver]

Also having ts_col as optional pretty much forces the users to use keyword arguments in the tick case, which may not be a bad thing but then it is not consistent with the time case.

[chipkent]

@jmao-denver thoughts on the argument ordering vs other methods in this module? e.g. did we do ema_tick_decay in the wrong order?

[jmao-denver]

The parameter ordering of ema_tick_decay/ema_time_decay matches the Java ones more closely, which is nice. But it is not something we can do with rolling sums. I have no objection to making them consistent if you feel strongly about it. Even though it is a breaking change, I doubt we'll affect users so much as our own docs/examples.

[lbooker42]

I also would prefer a closer match with Java/Groovy for rolling_sum, but I think the argument is about optional parameter(s). If we want to harmonize, it makes as much sense to me to update the Java signatures and move these optional parameters to the end of the signature.

[deephaven-internal]

Labels indicate documentation is required. Issues for documentation have been opened:

How-to: https://github.com/deephaven/deephaven.io/issues/2155
Conceptual: https://github.com/deephaven/deephaven.io/issues/2158
Reference: https://github.com/deephaven/deephaven.io/issues/2156
Blog: https://github.com/deephaven/deephaven.io/issues/2157