BUG: Index._searchsorted_monotonic(..., side='right') returns the left side position for monotonic decreasing indexes #17272

jschendel · 2017-08-17T18:55:50Z

closes BUG: Index._searchsorted_monotonic(..., side='right') returns the left side position for monotonic decreasing indexes #17271
closes BUG: IntervalIndex scalar lookup raises for included right endpoints when sorted decreasing. #16417
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

I didn't add a whatsnew entry for the Index._searchsorted_monotonic fix, since it looks like past convention has been to not add whatsnew entries for private methods. Happy to add a whatsnew entry if I'm mistaken though. I did add a whatsnew entry for the downstream effect on IntervalIndex.

gfyoung · 2017-08-17T19:00:47Z

since it looks like past convention has been to not add whatsnew entries for private methods.

That is correct.

jreback · 2017-08-18T00:40:11Z

pandas/tests/indexes/test_base.py

+    def test_searchsorted_monotonic(self):
+        # GH17271
+        idx_inc = Index([0, 2, 4])
+        idx_dec = Index([4, 2, 0])


would be better to move these to common.py/Base as they be tested generically.

jreback · 2017-08-18T00:41:10Z

pandas/tests/indexing/test_interval.py

@@ -39,6 +42,9 @@ def test_loc_with_scalar(self):
        expected = s.iloc[2:5]
        tm.assert_series_equal(expected, s.loc[s >= 2])

+        # test endpoint of non-overlapping monotonic decreasing (GH16417)


I would instead make a specific test of this (also test increasing monotonic), even better would be to parametrize over closed (bonus points)

codecov · 2017-08-21T01:47:30Z

Codecov Report

Merging #17272 into master will decrease coverage by 50.77%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #17272       +/-   ##
===========================================
- Coverage   91.03%   40.25%   -50.78%     
===========================================
  Files         162      162               
  Lines       49567    49540       -27     
===========================================
- Hits        45123    19944    -25179     
- Misses       4444    29596    +25152

Flag	Coverage Δ
#multiple	`?`
#single	`40.25% <ø> (-0.05%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/base.py	`56.35% <ø> (-39.59%)`	⬇️
pandas/tools/hashing.py	`0% <0%> (-100%)`	⬇️
pandas/io/sas/sas_constants.py	`0% <0%> (-100%)`	⬇️
pandas/lib.py	`0% <0%> (-100%)`	⬇️
pandas/parser.py	`0% <0%> (-100%)`	⬇️
pandas/tslib.py	`0% <0%> (-100%)`	⬇️
pandas/io/json/json.py	`0% <0%> (-100%)`	⬇️
pandas/io/formats/style.py	`0% <0%> (-100%)`	⬇️
pandas/types/concat.py	`0% <0%> (-100%)`	⬇️
pandas/io/sas/sas7bdat.py	`0% <0%> (-90.77%)`	⬇️
... and 112 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 58d8729...436852e. Read the comment docs.

codecov · 2017-08-21T01:47:31Z

Codecov Report

Merging #17272 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #17272      +/-   ##
==========================================
- Coverage   91.15%   91.14%   -0.02%     
==========================================
  Files         163      163              
  Lines       49591    49591              
==========================================
- Hits        45207    45201       -6     
- Misses       4384     4390       +6

Flag	Coverage Δ
#multiple	`88.93% <ø> (ø)`	⬆️
#single	`40.25% <ø> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/base.py	`96.29% <ø> (ø)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.72% <0%> (-0.1%)`	⬇️
pandas/core/indexes/datetimes.py	`95.52% <0%> (+0.09%)`	⬆️
pandas/core/indexes/datetimelike.py	`96.88% <0%> (+0.22%)`	⬆️
pandas/core/indexes/range.py	`92.59% <0%> (+0.3%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20fee85...41e59d7. Read the comment docs.

jschendel · 2017-08-21T01:51:49Z

Notes regarding the review changes I made:

Removed some redundant checks from test_loc_with_scalar and test_getitem_with_scalar in test_interval.py, as these checks are now being handled by test_nonoverlapping_monotonic (the index used in the two former tests is nonoverlapping monotonic increasing).
Added some monotonic decreasing indexes to setup_method to various test classes within in order to ensure that monotonic decreasing indexes are fully tested by common.py/test_searchsorted_monotonic (and other tests).
- Had to change tm.assert_numpy_array_equal to tm.equalContents in common.py/test_difference_base. Introducing a decreasing sequence led to the test failing with tm.assert_numpy_array_equal due to different sorting of the result and expected answer. If keeping tm.assert_numpy_array_equal is preferable, the arrays could be sorted prior to using it.
- Did not add a monotonic decreasing index to test_range, as it currently fails common.py/test_intersection_base due to the issue described in BUG: Intersection of RangeIndexes fails when the step sign is negative #17296 (will add when I submit a PR for this issue).

jreback · 2017-08-21T01:58:15Z

Had to change tm.assert_numpy_array_equal to tm.equalContents in common.py/test_difference_base. Introducing a decreasing sequence led to the test failing with tm.assert_numpy_array_equal due to different sorting of the result and expected answer. If keeping tm.assert_numpy_array_equal is preferable, the arrays could be sorted prior to using it.

yes, pls just sort them if need; we don't use tm.equalContents (actually should remove that)

Did not add a monotonic decreasing index to test_range, as it currently fails common.py/test_intersection_base due to the issue described in #17296 (will add when I submit a PR for this issue).

simply add an xfailing test (with the issue number).

jschendel · 2017-08-21T02:32:40Z

Okay, made those additional changes. ~~Any idea why codecov is saying this will decrease coverage by 50%? I don't think I made any substantial changes that would cause that. Just a glitch?~~

EDIT: Nevermind, looks like codecov is fixed.

jschendel · 2017-08-23T03:31:58Z

ping @jreback : made review related updates and did a rebase to resolve merge conflicts

jreback · 2017-08-23T11:56:54Z

doc/source/whatsnew/v0.21.0.txt

@@ -354,6 +354,7 @@ Indexing
 - Avoids ``IndexError`` when passing an Index or Series to ``.iloc`` with older numpy (:issue:`17193`)
 - Allow unicode empty strings as placeholders in multilevel columns in Python 2 (:issue:`17099`)
 - Bug in ``.iloc`` when used with inplace addition or assignment and an int indexer on a ``MultiIndex`` causing the wrong indexes to be read from and written to (:issue:`17148`)
+- Bug in ``IntervalIndex`` where performing a scalar lookup fails for included right endpoints of non-overlapping monotonic decreasing indexes (:issue:`16417`)


can you add both issue numbers

jreback · 2017-08-23T11:58:39Z

pandas/tests/indexes/common.py

+            value = index[0]
+
+            if index.is_monotonic_increasing or index.is_monotonic_decreasing:
+                assert index._searchsorted_monotonic(value, side='left') == 0


can you test .searchsorted() as well for the various cases

jreback · 2017-08-23T12:01:50Z

pandas/tests/indexing/test_interval.py

@@ -67,6 +48,41 @@ def test_getitem_with_scalar(self):
        expected = s.iloc[2:5]
        tm.assert_series_equal(expected, s[s >= 2])

+    @pytest.mark.parametrize('direction, closed',
+                             product(('increasing', 'decreasing'),
+                                     ('left', 'right', 'neither', 'both')))


@zfrenchee if you'd have a look here

jschendel · 2017-08-25T05:22:22Z

Made the requested changes. A couple comments:

Only tested .searchsorted for monotonic increasing, since that's the underlying assumption.
Added an index with repeats to test_base to better test side='right' for _searchsorted_monotonic and .searchsorted.
- Required a modifying test_get_indexer_consistency to check uniqueness in one place where uniqueness was assumed.
Moved the xfail for test_intersection_base to only occur for RangeIndex, since that's the only type of index that should fail.

jreback

minor request. lgtm. ping on green.

jreback · 2017-08-25T11:27:49Z

pandas/tests/indexes/common.py

+            if index.is_unique:
+                indexer = index.get_indexer(index[0:2])
+                assert isinstance(indexer, np.ndarray)
+                assert indexer.dtype == np.intp


can you add what the result if not index.is_unique as well

jschendel · 2017-08-25T21:24:04Z

ping @jreback : checks are green

Made one small additional change to the index.is_unique case. Since get_indexer is implemented slightly differently for CategoricalIndex relative to other indexes, they're actually fine even if the values in the index aren't unique, so they fell into the index.is_unique case regardless of uniqueness.

jschendel · 2017-08-29T20:35:46Z

ping @jreback : green again after rebasing to address the merge conflict.

Fixed an issue where Index._searchsorted_monotonic(..., side='right') returns the left side position for monotonic decreasing indexes. Issue had a downstream effect on scalar lookups in IntervalIndex as well.

Removed xfail, as the issue impacting the failing test has been resolved. Rebase to address merge conflicts.

jreback · 2017-09-07T11:49:37Z

thanks @jschendel nice fixes! keep em coming!

…t side position for monotonic decreasing indexes (pandas-dev#17272)

jbrockmendel · 2020-12-06T03:59:04Z

pandas/tests/indexes/common.py

-            indexer = index.get_indexer(index[0:2])
-            assert isinstance(indexer, np.ndarray)
-            assert indexer.dtype == np.intp
+            if index.is_unique or isinstance(index, CategoricalIndex):


@jschendel why is CategoricalIndex excluded here?

gfyoung added the Indexing Related to indexing on series/frames, not to indexes themselves label Aug 17, 2017

jreback requested changes Aug 18, 2017

View reviewed changes

jschendel force-pushed the fix-idx-searchsortedmonotonic branch from 6378a27 to 436852e Compare August 21, 2017 01:47

jschendel force-pushed the fix-idx-searchsortedmonotonic branch from b4ea216 to 1002aaa Compare August 23, 2017 02:16

jreback added this to the 0.21.0 milestone Aug 23, 2017

jreback requested changes Aug 23, 2017

View reviewed changes

jreback reviewed Aug 23, 2017

View reviewed changes

jschendel force-pushed the fix-idx-searchsortedmonotonic branch 2 times, most recently from 38b4971 to 361face Compare August 25, 2017 05:06

jreback requested changes Aug 25, 2017

View reviewed changes

jschendel force-pushed the fix-idx-searchsortedmonotonic branch from 85ca734 to 1911860 Compare August 29, 2017 15:34

jschendel mentioned this pull request Aug 30, 2017

BUG: intersection of decreasing RangeIndexes #17374

Merged

4 tasks

jschendel added 6 commits September 6, 2017 20:23

FIX: Index._searchsorted_monotonic for decreasing indexes

b72147e

Fixed an issue where Index._searchsorted_monotonic(..., side='right') returns the left side position for monotonic decreasing indexes. Issue had a downstream effect on scalar lookups in IntervalIndex as well.

Review related changes

2125c43

Additional review changes

b857aca

Test searchsorted and misc changes

783f38f

Verify non-unique index

e078f4c

Rebase and remove xfail

41e59d7

Removed xfail, as the issue impacting the failing test has been resolved. Rebase to address merge conflicts.

jschendel force-pushed the fix-idx-searchsortedmonotonic branch from 1911860 to 41e59d7 Compare September 7, 2017 02:37

jreback approved these changes Sep 7, 2017

View reviewed changes

jreback merged commit 3a291bb into pandas-dev:master Sep 7, 2017

jschendel deleted the fix-idx-searchsortedmonotonic branch September 7, 2017 14:30

jbrockmendel pushed a commit to jbrockmendel/pandas that referenced this pull request Sep 10, 2017

BUG: Index._searchsorted_monotonic(..., side='right') returns the lef…

2be1405

…t side position for monotonic decreasing indexes (pandas-dev#17272)

alanbato pushed a commit to alanbato/pandas that referenced this pull request Nov 10, 2017

BUG: Index._searchsorted_monotonic(..., side='right') returns the lef…

8675197

…t side position for monotonic decreasing indexes (pandas-dev#17272)

No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017

BUG: Index._searchsorted_monotonic(..., side='right') returns the lef…

55a512f

…t side position for monotonic decreasing indexes (pandas-dev#17272)

jbrockmendel reviewed Dec 6, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Index._searchsorted_monotonic(..., side='right') returns the left side position for monotonic decreasing indexes #17272

BUG: Index._searchsorted_monotonic(..., side='right') returns the left side position for monotonic decreasing indexes #17272

jschendel commented Aug 17, 2017

gfyoung commented Aug 17, 2017

jreback Aug 18, 2017

jreback Aug 18, 2017

codecov bot commented Aug 21, 2017

codecov bot commented Aug 21, 2017 •

edited

Loading

jschendel commented Aug 21, 2017

jreback commented Aug 21, 2017

jschendel commented Aug 21, 2017 •

edited

Loading

jschendel commented Aug 23, 2017

jreback Aug 23, 2017

jreback Aug 23, 2017

jreback Aug 23, 2017

jschendel commented Aug 25, 2017

jreback left a comment

jreback Aug 25, 2017

jschendel commented Aug 25, 2017

jschendel commented Aug 29, 2017

jreback commented Sep 7, 2017

jbrockmendel Dec 6, 2020

BUG: Index._searchsorted_monotonic(..., side='right') returns the left side position for monotonic decreasing indexes #17272

BUG: Index._searchsorted_monotonic(..., side='right') returns the left side position for monotonic decreasing indexes #17272

Conversation

jschendel commented Aug 17, 2017

gfyoung commented Aug 17, 2017

jreback Aug 18, 2017

Choose a reason for hiding this comment

jreback Aug 18, 2017

Choose a reason for hiding this comment

codecov bot commented Aug 21, 2017

Codecov Report

codecov bot commented Aug 21, 2017 • edited Loading

Codecov Report

jschendel commented Aug 21, 2017

jreback commented Aug 21, 2017

jschendel commented Aug 21, 2017 • edited Loading

jschendel commented Aug 23, 2017

jreback Aug 23, 2017

Choose a reason for hiding this comment

jreback Aug 23, 2017

Choose a reason for hiding this comment

jreback Aug 23, 2017

Choose a reason for hiding this comment

jschendel commented Aug 25, 2017

jreback left a comment

Choose a reason for hiding this comment

jreback Aug 25, 2017

Choose a reason for hiding this comment

jschendel commented Aug 25, 2017

jschendel commented Aug 29, 2017

jreback commented Sep 7, 2017

jbrockmendel Dec 6, 2020

Choose a reason for hiding this comment

codecov bot commented Aug 21, 2017 •

edited

Loading

jschendel commented Aug 21, 2017 •

edited

Loading