Skip to content

Conversation

@Yikun
Copy link
Member

@Yikun Yikun commented Sep 8, 2022

What changes were proposed in this pull request?

Refactor expanding and rolling test for function with input

Why are the changes needed?

Refactor expanding and rolling test for function with input:

# Before
self._test_groupby_rolling_func("count")

# After
# str can be accept
self._test_groupby_rolling_func("count")
# Can also accept lambda to support more func style
self._test_groupby_expanding_func(
    lambda x: x.quantile(0.5), lambda x: x.quantile(0.5, "lower")
)

Does this PR introduce any user-facing change?

No

How was this patch tested?

  • CI passed
  • cherry-pick: e22ee1c and test manually.

@Yikun
Copy link
Member Author

Yikun commented Sep 9, 2022

cc @zhengruifeng @HyukjinKwon

@Yikun Yikun marked this pull request as draft September 9, 2022 06:28
@Yikun Yikun marked this pull request as ready for review September 9, 2022 07:34
Copy link
Contributor

@itholic itholic left a comment

Choose a reason for hiding this comment

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

Looks good otherwise

Comment on lines +101 to +102
if not pd_func:
pd_func = ps_func
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I got a bit confusion for this part.

Could you tell one example that considers this case?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's that it uses the same function if pd_func is omitted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, just like: self._test_groupby_rolling_func("count").

We add ps_func and pd_func separately for some case like: self._test_groupby_expanding_func(lambda x:x.quantile(0.5), lambda x: x.quantile(0.5, "lower")), pd_func and ps_func is different.

@HyukjinKwon
Copy link
Member

Merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants