[REVIEW] Rolling window C++ API #732
Conversation
There was a problem hiding this comment.
My review just suggests block comment improvements. Jake's review critiques the API.
My main concern is choosing to write NaN in the case where Pandas writes NA. These are not the same in my view. We should probably use the valid bit mask for this and just invalidate the element rather than storing NaN.
nsakharnykh
changed the title
|
@harrism & @jrhemstad can you give it another pass to make sure your comments are addressed? The plan is to finalize the API and merge, then I'll work on a separate PR for the implementation. |
|
@kkraus14 can someone from the Python devs review this, hopefully someone who would be working on the bindings, just to make sure there are no surprises there? |
nsakharnykh
changed the title
cpp/include/cudf/functions.h
Outdated
| /* --------------------------------------------------------------------------* | ||
| * @brief Computes the rolling window function of the values in a column. | ||
| * | ||
| * This matches Pandas' API for DataFrame.rolling with a couple notable |
There was a problem hiding this comment.
I think it's a good idea to have a self-contained description of what a function does rather than relying on someone to know what the Pandas DataFrame.rolling is.
It's fine to keep the documentation of what the deviations from the Pandas API are.
There was a problem hiding this comment.
I remember a while back seeing a big long list of all the stuff we wanted to support with rolling functions and this seems to be missing quite a bit of it. Even if we do it column by column we need to be able to do things like partitioned windows based not just on window size.
| * | ||
| * --------------------------------------------------------------------------*/ | ||
| gdf_error gdf_rolling_window(gdf_column *output_col, | ||
| const gdf_column *input_col, |
There was a problem hiding this comment.
Usually rolling operations are done in groups as opposed to a column at a time.
There was a problem hiding this comment.
I guess in pandas it might be different but windows are often combined in many data operations across multiple values
There was a problem hiding this comment.
Even if windows are executed across multiple values, these are independent operations and gdf_rolling_window can be called multiple times. I do see some value in providing an API that can handle multiple columns, which can benefit kernel launch timings, but otherwise there is no reuse of data between the columns. To avoid complicating the initial API, I would postpone this feature to the next version of the API, or until we get a realistic case that suffers from kernel launch latencies for rolling windows. Sounds good?
| * --------------------------------------------------------------------------*/ | ||
| gdf_error gdf_rolling_window(gdf_column *output_col, | ||
| const gdf_column *input_col, | ||
| gdf_size_type window, |
There was a problem hiding this comment.
What about partitioned windows? windows that are based on another columns value for example as opposed to based on fixed size? this is common when users dont have fixed number of events
There was a problem hiding this comment.
Updated in 4f61fee to address this
|
@harrism and @felipeblazing I updated the API, I believe it's ready to merge, let me know if you have further comments |
|
rerun tests |
nsakharnykh
added
5 - Ready to Merge
|
If @felipeblazing is happy with this API now, I think you can proceed with implementation. I think the next step is to create a branch from this branch to do the implementation in, so you can start a new PR with the full PR, and close this one out. |
|
@felipeblazing please answer whether you are now happy with the API after |
harrism
removed
the
5 - Ready to Merge
|
I am thrilled and excited at the changes and do so humbly wish to approve it as well. |
|
@nsakharnykh I'm closing this PR. You can branch from this branch and open a new PR with the implementation for issue #926, or if you prefer you can reopen this PR and rename it and add implementation. |
Posting to review the proposed API first, then I'll work on a GPU implementation (will be a separate PR).
This PR introduces a C++ API for rolling windows. It mirrors Pandas'
DataFrame.rollingAPI with some minor changes - seegdf_rolling_windowfor more detail.Also see #714 - pure Python implementation of rolling windows using numba kernels