TST: groupby of idxmax/min with mixed types #40795
Conversation
bsun94
commented
Apr 6, 2021
bsun94
commented
- closes TST: groupby of idxmax/min with mixed types #40346
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry - didn't think this was necessary, just added a test case to a solved problem.
…cel file, and fixed typo via pandas-dev#38990
…eets.rst Co-authored-by: Aidan Feldman <[email protected]>
…st_idxmax_mixed_dtypes for pandas-dev#40346
| @@ -1025,6 +1025,27 @@ def test_idxmax_mixed_dtype(self): | |||
| expected = Series([0, 2, 1, 2], index=[1, 2, 3, 4]) | |||
| tm.assert_series_equal(result, expected) | |||
|
|
|||
| # with convert dtypes | |||
There was a problem hiding this comment.
Please make a new test for this.
Could you try a parametrization?
There was a problem hiding this comment.
sure thing, I'll separate them out and use parametrization
| @@ -1025,6 +1025,46 @@ def test_idxmax_mixed_dtype(self): | |||
| expected = Series([0, 2, 1, 2], index=[1, 2, 3, 4]) | |||
| tm.assert_series_equal(result, expected) | |||
|
|
|||
| @pytest.mark.parametrize( | |||
| "ID, value, expected_index, expected_value", | |||
| [([100, 100, 100, 200, 200, 200], [0, 0, 0, 1, 2, 0], [100, 200], [0, 4])], | |||
There was a problem hiding this comment.
Please parametrize over ecpected values and the applied function only, you will need only one test then
There was a problem hiding this comment.
thanks @phofl , will redo and push
jreback
changed the title
jreback
added
Groupby
Testing
| } | ||
| ) | ||
|
|
||
| df = df.convert_dtypes().groupby("ID") |
There was a problem hiding this comment.
Instead of using convert_dtypes, better to just specify dtype="Int64" in DataFrame construction.
There was a problem hiding this comment.
I'd just like to confirm on this point - in the discussion I had with Marco in issue #40346, I understood the spirit of this new check as to replicate the conditions under which the original bug was found (in using convert_dtypes before idxmax) to make sure the bug was fixed. Would it not be preferable, in that case, to keep using the method which caused the original bug?
There was a problem hiding this comment.
Hi @rhshadrach , I'd just like to follow-up on the above - please let me know, and would be happy to push through all the updates together
There was a problem hiding this comment.
Thanks for the reminder here, sorry it fell off my radar. From #40346 (comment) I believe @MarcoGorelli is just looking to create a DataFrame with nullable integers. My suggested change accomplishes this, but with using less of the pandas API. In general, it's desirable for tests to use as little of the API (besides what they are testing!) as possible.
There was a problem hiding this comment.
I see, thanks @rhshadrach! I'll push the changes through
| "func_name, expected_value", | ||
| [("idxmax", [0, 4]), ("idxmin", [0, 5])], | ||
| ) | ||
| def test_idxmax_idxmin_convert_dtypes(self, func_name, expected_value): |
There was a problem hiding this comment.
Just for consistency with other tests, and you rename func_name -> op
| df = df.convert_dtypes().groupby("ID") | ||
| func = getattr(df, func_name) | ||
|
|
||
| result = func() |
There was a problem hiding this comment.
Can just call in the line above, no need for the intermediate func.
|
thanks @bsun94 |
