Skip to content

CLN: Unify Window._apply_window and Rolling._apply functions #27403

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 15 commits into from
Jul 31, 2019
Merged

CLN: Unify Window._apply_window and Rolling._apply functions #27403

merged 15 commits into from
Jul 31, 2019

Conversation

ihsansecer
Copy link
Contributor

  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff

roll_window function is split into two to enable implementing other weighted functions e.g. var, std (ref #26597)

jreback
jreback requested changes Jul 15, 2019
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be possible to make this very generic
as well already support passing functions in _applly for Rolling
can u see how much code can be consilidated here

@ihsansecer
Copy link
Contributor Author

@jreback ok kept original function unchanged while calling it in separate functions. need to pass str functions in _apply_window also. _apply doesn't implement weights

jreback
jreback reviewed Jul 15, 2019
View reviewed changes
jreback
jreback reviewed Jul 15, 2019
View reviewed changes
@jreback jreback added the Reshaping Concat, Merge/Join, Stack/Unstack, Explode label Jul 15, 2019
@jreback jreback mentioned this pull request Jul 15, 2019
Closed
@ghost
Copy link

ghost commented Jul 16, 2019

in the spirit of #27409, any objections to renaming to roll_weighted_window_foo or some such? it's confusing having Window, Rolling and roll_window, where the latter two can both do ops like sum/mean.

@ihsansecer
Copy link
Contributor Author

In favour of renaming it to window_foo or weighted_roll_foo

@ihsansecer ihsansecer changed the title CLN: Split roll_window function CLN: Merge Window._apply_window and Rolling._apply functions Jul 16, 2019
ihsansecer
ihsansecer commented Jul 16, 2019
View reviewed changes
jreback
jreback reviewed Jul 18, 2019
View reviewed changes
@ihsansecer ihsansecer changed the title CLN: Merge Window._apply_window and Rolling._apply functions CLN: Unify Window._apply_window and Rolling._apply functions Jul 18, 2019
@ihsansecer
Copy link
Contributor Author

It seems some network tests have failed

@ihsansecer
Lower
6494a1e 
Rerun ci
jreback
jreback requested changes Jul 20, 2019
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks really good, some comments.

pandas/core/window.py
window : int/array, default to _get_window()
center : bool, default to self.center
check_minp : function, default to _use_window

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add what kwargs is here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we modify kwargs (or copy of it) to remove window function (passed to _get_window) kwargs first. window rolling functions (passed to _get_roll_func) don't take kwargs but when they do it will be a problem

@jreback jreback added the Clean label Jul 20, 2019
ghost
ghost reviewed Jul 20, 2019
View reviewed changes
ihsansecer
ihsansecer commented Jul 21, 2019
View reviewed changes
pandas/core/window.py
@@ -374,9 +398,12 @@ def _apply(
func : str/callable to apply
name : str, optional
name of this function
window : int/array, default to _get_window()
window : int/str, default to _get_window()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't see any passing array to _apply. This seems to be an offset or int same as in rolling function

ihsansecer
ihsansecer commented Jul 21, 2019
View reviewed changes
pandas/core/window.py Outdated
index_as_array = self._get_index()

results = []
exclude = []
Copy link
Contributor Author

@ihsansecer ihsansecer Jul 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pandas/core/window.py:426: error: Need type annotation for 'exclude'

not sure what confuses mypy to require type for exclude (this is the reason of ci fail)

@ghost
Copy link

ghost commented Jul 24, 2019

Would this be a good opportunity to include a fix for issue #26462 ? Currently it looks like the win_type parameter is ignored in a df.goupby(...).rolling(..., win_type=xyz) aggregation

@jreback
Copy link
Contributor

jreback commented Jul 25, 2019

@ihsansecer pls merge master and update when you can, ping on green.

@jreback
Copy link
Contributor

jreback commented Jul 25, 2019

Would this be a good opportunity to include a fix for issue #26462 ? Currently it looks like the win_type parameter is ignored in a df.goupby(...).rolling(..., win_type=xyz) aggregation

@Connossor we try to keep 1 PR to 1 thing; this is a refactor only.

ihsansecer
ihsansecer commented Jul 29, 2019
View reviewed changes
pandas/core/window.py Outdated
@@ -387,7 +387,7 @@ def _apply(
center: Optional[bool] = None,
check_minp: Optional[Callable] = None,
**kwargs
) -> FrameOrSeries:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed because mypy was complaining

jreback
jreback approved these changes Jul 31, 2019
View reviewed changes
pandas/core/window.py
def _get_window(self, other=None):
def _get_window(self, other=None, **kwargs) -> int:
"""
Returns window lenght
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

@jreback jreback added this to the 1.0 milestone Jul 31, 2019
@jreback jreback merged commit 4ff0d61 into pandas-dev:master Jul 31, 2019
@ihsansecer ihsansecer deleted the split-win branch July 31, 2019 14:00
quintusdias pushed a commit to quintusdias/pandas_dev that referenced this pull request Aug 16, 2019
@ihsansecer @quintusdias
CLN: Unify Window._apply_window and Rolling._apply functions (pandas-…
4c35e33 
…dev#27403)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
Clean Reshaping Concat, Merge/Join, Stack/Unstack, Explode
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@ihsansecer @jreback