BUG: Pandas, CSV, and Parquet backends do not have inclusive preceding window bound

[hjoo]

According to documentation, ibis window preceding and following bounds are inclusive.

Pandas, CSV, and Parquet backends have exclusive preceding window boundaries.

Example (taken from ibis/all/test_window.py):

window = ibis.window(
        preceding=2,
        following=0,
        group_by=[alltypes.string_col],
        order_by=[alltypes.id],
    )

expr = alltypes.mutate(val=alltypes.double_col.sum().over(window))
result = expr.execute().set_index('id').sort_index()

result[['string_col', 'double_col', 'val']].groupby('string_col').get_group('1').head()
   string_col  double_col   val
id                             
1           1        10.1  10.1
11          1        10.1  20.2
21          1        10.1  20.2
31          1        10.1  20.2
41          1        10.1  20.2

expected[['string_col', 'double_col', 'val']].groupby('string_col').get_group('1').head()
   string_col  double_col   val
id                             
1           1        10.1  10.1
11          1        10.1  20.2
21          1        10.1  30.3
31          1        10.1  30.3
41          1        10.1  30.3

[hjoo]

Pandas backend behavior is also incorrect for ibis.trailing_window. The argument to ibis.trailing_window() is a preceding inclusive window bound according to the ibis docs. This is also how other sql backends behave. Currently the tests in pandas backend treat the argument as a window size.

Rather, this is the expected behavior:

ibis.trailing_window(preceding=2) is equivalent to pandas.DataFrame.rolling(3) where 3 is the window size.

ibis.trailing_window(preceding=0) is equivalent to pandas.DataFrame.rolling(1) where 1 is the window size.

So on and so forth.

[hjoo]

@jreback @toryhaavik: let me know if you disagree. I'll be working on a PR with updated tests and the behavior described above.

[jreback]

Rather, this is the expected behavior:

ibis.trailing_window(preceding=2) is equivalent to pandas.DataFrame.rolling(3) where 3 is the window size.

ibis.trailing_window(preceding=0) is equivalent to pandas.DataFrame.rolling(1) where 1 is the window size.

This just seems wrong. Why would you expect this? or maybe I am misunderstanding the meaning of preceding. the argument to rolling is the window size.

[hjoo]

That's what's expected in other backends. The argument to pandas rolling is window size. The argument to ibis trailing_window is not size but an inclusive preceding row bound. 0 indicates current row, 1 indicates row before, etc.

[jreback]

ok can u make that clear in comments /
doc strings

also i think a release note should indicate this is an api breaking change

[hjoo]

Updated docs and release notes in PR #2009. Let me know if I used the right convention.

Re: window size vs window boundary - I agree it's a bit confusing, but I'd prefer to separate the issue from this one because I think that will need a longer API discussion. If we want to change the Ibis trailing_window API altogether to take a window size (like pandas) rather than preceding window bound, window(preceding=1, following=0) would no longer be equivalent to trailing_window(1).

[icexelloss]

@hjoo @jreback Can this be closed?

[hjoo]

Yep, will close.