BUG: 43909 - check monoticity of rolling groupby #44068
Conversation
|
Hello @jamesholcombe! 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 2021-12-22 13:40:13 UTC |
jamesholcombe
changed the title
|
@github-actions pre-commit |
|
This PR should close issue 43909. Previous behaviour of a rolling window on a groupby was inconsistent with behaviour of rolling window on a dataframe. The former raises an error for non-monotonic "on". I have amended the latter to match this behaviour. |
|
@github-actions pre-commit |
pandas/tests/window/test_rolling.py
Outdated
| def test_groupby_rolling_non_monotonic(): | ||
| # GH 43909 | ||
| shuffled = [3, 0, 1, 2] | ||
| sec = 1_000_000_000 |
There was a problem hiding this comment.
can you use a smaller number here
| result = df2.groupby("A").rolling("4s", on="B").C.mean() | ||
| tm.assert_series_equal(result, expected) | ||
| with pytest.raises(ValueError, match=r".* must be monotonic"): | ||
| df.groupby("A").rolling("4s", on="B").C.mean() |
There was a problem hiding this comment.
can you also add back the .sort_values(...) then a successful groupby / rolling
There was a problem hiding this comment.
the successful groupby with rolling is the test above
There was a problem hiding this comment.
but its not the same as what you are doing. it fails, you sort then it works. this needs testing.
There was a problem hiding this comment.
agreed - amended in last push
|
@github-actions pre-commit |
|
Whats New: Value Error now raised when a non-monotonic time series passed to "_on" attribute of a rolling groupby. This is to match the error raised for a rolling window on a dataframe. Tests have been added to confirm expected behaviour. |
|
@github-actions pre-commit |
|
@github-actions pre-commit |
|
Hi Jeff, |
|
@github-actions pre-commit |
|
NOTE: master branch contains breaking changes so have not merged most recent. |
what does this mean? |
|
still needs a whatsnew note and merge master |
1 similar comment
|
still needs a whatsnew note and merge master |
|
@github-actions pre-commit |
1 similar comment
|
@github-actions pre-commit |
|
Find whats new note at top of PR. master merged. |
|
@github-actions pre-commit |
|
whatsnew note goes in the doc/ folder under 1.4.0 for bug fixes |
|
pre-commit is failing, pls merge master and ping on green. |
|
pre commit now passing |
|
thanks @jamesholcombe |
closes BUG: Using
df.groupby(..).rolling(..)on non-monotonic timestamp column does not raise an exception #43909tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry
Whats new:
Exception now raised when non-monotonic data passed to rolling groupby
Ammendment of tests to reflect this changed behaviour
Addition of test for the new behaviour