-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
TST: groupby apply called multiple times #34897
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.
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
TST: groupby apply called multiple times #34897
Conversation
Hello @Rohith295! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-06-20 14:58:22 UTC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Rohith295 ! Seems there's some linting failures, can you run black
and flake8
against your changed file?
pandas/tests/groupby/test_apply.py
Outdated
def test_apply_function_called_count(capsys): | ||
# GH: 31111 | ||
# groupby-apply need to execute len(set(group_by_columns)) times | ||
# `https://github.com/pandas-dev/pandas/issues/31111` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you already have the issue number you don't need the link as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. I understood that. Will make sure not to repeat this again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed accordingly. @MarcoGorelli
pandas/tests/groupby/test_apply.py
Outdated
# `https://github.com/pandas-dev/pandas/issues/31111` | ||
|
||
|
||
function_called_count = 2 # Number of times `apply` should call a function for the current test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps call this expected
, and call capsys.readouterr().out.count("function_called")
result
, and at the end do assert result == expected
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I will make it more readable.
pandas/tests/groupby/test_apply.py
Outdated
@@ -974,3 +974,24 @@ def test_apply_function_with_indexing_return_column(): | |||
result = df.groupby("foo1", as_index=False).apply(lambda x: x.mean()) | |||
expected = DataFrame({"foo1": ["one", "three", "two"], "foo2": [3.0, 4.0, 4.0]}) | |||
tm.assert_frame_equal(result, expected) | |||
|
|||
|
|||
def test_apply_function_called_count(capsys): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this near test_group_apply_once_per_group and indicate the same issue number (as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and rename these test to test_group_apply_once_per_group2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright @jreback . Will change it accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should remove GH: 31111
and use # GH2936, GH7739, GH10519, GH2656, GH12155, GH20084, GH21417
@jreback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can add this issue number as well is ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright. fixed accordingly
this is very similar to these issues |
lgtm ping on green. |
thanks @Rohith295 |
closes #31111
xref #24748
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff