BUG: do not crash on a callable grouper returning tuples #22279
Conversation
|
Hello @toobaz! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on August 12, 2018 at 08:48 Hours UTC |
toobaz
force-pushed
the
gb_func_tuples_22257
branch
from
ba3fc4c to
69e696f
Compare
Codecov Report
@@ Coverage Diff @@
## master #22279 +/- ##
==========================================
- Coverage 92.08% 92.07% -0.01%
==========================================
Files 169 169
Lines 50694 50699 +5
==========================================
+ Hits 46682 46683 +1
- Misses 4012 4016 +4
Continue to review full report at Codecov.
|
| gb = df.groupby(by=func, sort=False) | ||
|
|
||
| name, expected = list(gb)[0] | ||
| result = gb.get_group(name) |
There was a problem hiding this comment.
Can you be more explicit about what should be returned here? I think retrieving the element from the GroupBy object and subsequently passing that to get_group could potentially hide bugs in the future
| @@ -508,6 +508,15 @@ def _get_grouper(obj, key=None, axis=0, level=None, sort=True, | |||
| warnings.warn(msg, FutureWarning, stacklevel=5) | |||
| key = list(key) | |||
|
|
|||
| if callable(key) or isinstance(key, dict): | |||
There was a problem hiding this comment.
Is the isinstance(key, dict) new here? If so probably want a test case to cover
There was a problem hiding this comment.
(done - expanded the test)
There was a problem hiding this comment.
can you integrate this with the below (e.g. i/elif maybe enough)?
can add a top-levevel comment on what is going on here
| @@ -346,7 +360,7 @@ def test_groupby_grouper_f_sanity_checked(self): | |||
| # when the elements are Timestamp. | |||
| # the result is Index[0:6], very confusing. | |||
|
|
|||
| pytest.raises(AssertionError, ts.groupby, lambda key: key[0:6]) | |||
| pytest.raises(ValueError, ts.groupby, lambda key: key[0:6]) | |||
There was a problem hiding this comment.
What is the reason for this change?
There was a problem hiding this comment.
The single callable/mapping case used to follow the same code path as lists of callables/mappings, while it now follows the same code path as e.g. lists of Series. The two happen to check the length of the grouper in different ways (the former should probably be changed to ValueError too, but we might decide to remove it altogether in #22278).
There was a problem hiding this comment.
Notice this test will also change when we remove the absurd DatetimeIndex.map implementation (see #22312 ) but that is presumably not happening in a bugfix release.
toobaz
force-pushed
the
gb_func_tuples_22257
branch
from
69e696f to
9123f7d
Compare
gfyoung
added
Groupby
Regression
| @@ -20,10 +20,11 @@ and bug fixes. We recommend that all users upgrade to this version. | |||
| Fixed Regressions | |||
| ~~~~~~~~~~~~~~~~~ | |||
|
|
|||
| - Constructing a DataFrame with an index argument that wasn't already an | |||
| - Constructing a :class:`DataFrame` with an index argument that wasn't already an | |||
| @@ -508,6 +508,15 @@ def _get_grouper(obj, key=None, axis=0, level=None, sort=True, | |||
| warnings.warn(msg, FutureWarning, stacklevel=5) | |||
| key = list(key) | |||
|
|
|||
| if callable(key) or isinstance(key, dict): | |||
There was a problem hiding this comment.
can you integrate this with the below (e.g. i/elif maybe enough)?
can add a top-levevel comment on what is going on here
|
@toobaz can you merge master and move the doc-note to 0.24.0 |
|
closing as stale, but pls reopen if can continue |
git diff upstream/master -u -- "*.py" | flake8 --diffNotice I didn't have time to test which commit caused the regression.