BUG: Fix Series.nlargest for integer boundary values #21432
Conversation
jschendel
added
Bug
Dtype Conversions
| result = getattr(s, method)(3) | ||
| expected_idxr = [0, 1, 2] if method == 'nsmallest' else [3, 2, 1] | ||
| expected = s.loc[expected_idxr] | ||
| tm.assert_series_equal(result, expected) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@jschendel : Not a problem. Just wait for #21433 and rebase. |
|
#21433 has been merged. Feel free to rebase to add your |
jschendel
force-pushed
the
nlargest-integer-boundary
branch
from
c52bcf7 to
73595b3
Compare
Codecov Report
@@ Coverage Diff @@
## master #21432 +/- ##
==========================================
+ Coverage 91.89% 91.9% +<.01%
==========================================
Files 153 153
Lines 49604 49606 +2
==========================================
+ Hits 45584 45588 +4
+ Misses 4020 4018 -2
Continue to review full report at Codecov.
|
| @@ -1946,6 +1946,10 @@ def test_mode_sortwarning(self): | |||
|
|
|||
| class TestNLargestNSmallest(object): | |||
|
|
|||
| @pytest.fixture(params=['nlargest', 'nsmallest']) | |||
There was a problem hiding this comment.
would not object to moving this fixture to conftest (at a high level) and can replace usage in frame/test_analytics.py as well.
| @@ -2028,6 +2032,40 @@ def test_n(self, n): | |||
| expected = s.sort_values().head(n) | |||
| assert_series_equal(result, expected) | |||
|
|
|||
| def _check_nselect_boundary(self, vals, dtype, method): | |||
There was a problem hiding this comment.
name: assert_check_nselect_boundary
can be a module level function
| expected = s.loc[expected_idxr] | ||
| tm.assert_series_equal(result, expected) | ||
|
|
||
| @pytest.mark.parametrize('dtype', [ |
There was a problem hiding this comment.
I think we have a fixture for these types (if not, can you create one in conftest)
There was a problem hiding this comment.
@jreback : @jschendel brought up a good point in #21456 (comment) that we might have some intersection with this PR and #21456. How best to work that out?
There was a problem hiding this comment.
FWIW I've modified the fixtures I wrote to exactly match @gfyoung's. Were basically identical anyways. I've got all my review related changes ready, so can push whenever.
| vals = [min_val, min_val + 1, max_val - 1, max_val] | ||
| self._check_nselect_boundary(vals, dtype, method) | ||
|
|
||
| @pytest.mark.parametrize('dtype', ['float16', 'float32', 'float64']) |
There was a problem hiding this comment.
pls create a fixture (drop float16 we barely support this)
jschendel
force-pushed
the
nlargest-integer-boundary
branch
from
73595b3 to
fb9d532
Compare
|
Addressed review comments; copied @gfyoung's dtype fixtures where appropriate. Did some additional cleaning of |
|
thanks @jschendel |
|
can you create an issue to use these fixtures project wide? |
Created #21500 for this |
(cherry picked from commit ec5956e)
(cherry picked from commit ec5956e)
git diff upstream/master -u -- "*.py" | flake8 --diffAlso added some similar tests for float and datetimelike dtypes to ensure that the behavior is as desired.