-
-
Notifications
You must be signed in to change notification settings - Fork 19.4k
TST: groupby-reindex on DTI #33638
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-reindex on DTI #33638
Conversation
|
I think need to run black on the change to get CI to pass |
4ba0e25 to
256a584
Compare
256a584 to
8846135
Compare
done and now CI-linting passes |
pandas/tests/groupby/test_apply.py
Outdated
| srs_exp = pd.Series(values, index=indices, name="value") | ||
| srs_grouped = df.groupby("group").value.apply( | ||
| lambda x: x.reindex(np.arange(x.index.min(), x.index.max() + 1)) | ||
| ) |
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.
theres a lot going on here. can you break this up into multiple lines and comment on which one used to be a problem
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.
I wrote a seperate apply function and gave a little bit more background in the comments. Let me know if there is still somthing missing
pandas/tests/groupby/test_apply.py
Outdated
| srs_grouped = df.groupby("group").value.apply( | ||
| lambda x: x.reindex(np.arange(x.index.min(), x.index.max() + 1)) | ||
| ) | ||
| tm.assert_series_equal(srs_exp, srs_grouped) |
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.
can you call these expected and result
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.
done
pandas/tests/groupby/test_apply.py
Outdated
|
|
||
|
|
||
| def test_apply_reindex_values(): | ||
| # GH: 26209 |
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.
a few words about what the bug was would be hepful
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.
done
improved commentary
|
Hello @CloseChoice! 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-04-23 22:17:39 UTC |
|
thanks @CloseChoice |
Add test to check whether reindexing works correctly.
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff