TST: Add test for rolling window, see GH 34605 #34705
Conversation
|
Hello @gglanzani! 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-15 06:54:55 UTC |
gglanzani
changed the title
pandas/tests/window/test_rolling.py
Outdated
| df.rolling(1).apply(scaled_sum, raw=True, args=(2,)) | ||
|
|
||
| for grouping in groupings: | ||
| df.groupby(**grouping).rolling(1).apply(scaled_sum, raw=True, args=(2,)) |
There was a problem hiding this comment.
I think we can parameterize over raw; this doesn't need to be fixed.
I think we have a raw pytest fixture you can use.
pandas/tests/window/test_rolling.py
Outdated
|
|
||
| df = DataFrame(data={"X": range(5)}, index=[0, 0, 1, 1, 1]) | ||
|
|
||
| df.rolling(1).apply(scaled_sum, raw=True, args=(2,)) |
There was a problem hiding this comment.
I think this case is already tested (so you can remove)
pandas/tests/window/test_rolling.py
Outdated
|
|
||
| df.rolling(1).apply(scaled_sum, raw=True, args=(2,)) | ||
|
|
||
| for grouping in groupings: |
There was a problem hiding this comment.
We'll want to test the output of this operation
result = df.groupby(**grouping)...
expected = pd.DataFrame(...)
tm.assert_frame_equal(result, expected
There was a problem hiding this comment.
Also just simplify the parametrize to:
@pytest.mark.parametrize("groupings", [{"level": 0}, {"by": "X"}]
so we can remove the for loop here
|
@mroeschke Thanks for the review Matthew, I think I've addressed all your points. |
|
Looks good. I think our CI is having issues currently, so we can probably merge this once that is fixed. |
jreback
added
Testing
|
lgtm. yep can merge once CI is good. |
|
@gglanzani can you merge master? |
|
@WillAyd Done |
|
thanks @gglanzani |
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffDear maintainers,
I'm opening this PR to address issue #34605.
I've made the test conditional to Python 3.8 as I'm using the positional only syntax as strong check. If this is not desired, let me know and I will change the function to explicitly check for the length of*args.Edit: maybe the usage of
parametrizeis a bit exaggerated here.Edit2: Using Python 3.8 only features does not pleases the tests. Updating.