feat: add on parameter in dataframe.rolling() and dataframe.groupby.rolling()
#1556
Conversation
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
| window, | ||
| min_periods: int | None = None, | ||
| on: str | None = None, | ||
| closed: Literal["right", "left", "both", "neither"] = "right", |
There was a problem hiding this comment.
No code examples are added in this rolling method yet. Could you please help to fill the gaps here?
There was a problem hiding this comment.
Thanks! Maybe add more examples with supported parameters.
| to the size of the window. | ||
| on (str, optional): | ||
| For a DataFrame, a column label or Index level on which to calculate the rolling window, |
There was a problem hiding this comment.
Can you try the "Index level" cases here?
There was a problem hiding this comment.
Good catch. I removed index level here because I do not plan to support that at this time
0ed41e1 to
50df383
Compare
There was a problem hiding this comment.
LGTM overall with two nit comments. Thanks!
| window, | ||
| min_periods: int | None = None, | ||
| on: str | None = None, | ||
| closed: Literal["right", "left", "both", "neither"] = "right", |
There was a problem hiding this comment.
Thanks! Maybe add more examples with supported parameters.
| to the size of the window. | ||
| on (str, optional): | ||
| For a DataFrame, a column label or Index level on which to calculate the rolling window, |
There was a problem hiding this comment.
Should "Index level" be removed here too?
bigframes/core/window_spec.py
Outdated
| ordering: Tuple[orderings.OrderingExpression, ...] = tuple() | ||
| bounds: Union[RowsWindowBounds, RangeWindowBounds, None] = None | ||
| min_periods: int = 0 | ||
| on: str | None = None |
There was a problem hiding this comment.
hmm, not sure I understand this? I don't see why it should be part of the window spec? we already have "input column".
There was a problem hiding this comment.
Parameter removed. PTAL
b453a96 to
c27d0bc
Compare
| self._is_series = is_series | ||
| # The column ID that won't be aggregated on. | ||
| # This is equivalent to pandas `on` parameter in rolling() | ||
| self._skip_agg_column_id = skip_agg_column_id |
There was a problem hiding this comment.
I think this will eventually be a set in order to handle multiple columns for on?
There was a problem hiding this comment.
That would be unlikely. Both pandas and SQL allow only one column to be used for on (or its equivalent)
There was a problem hiding this comment.
Hmm, maybe not for on, but for grouping columns. We diverge a bit right now I think for some grouping cases. Anyways, can always change later if need be.
I didn't add this parameter for Series because it does seem to make much sense: Series doesn't have columns.