-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
BUG: IntervalIndex.get_loc/get_indexer wrong return value / error #25090
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 10 commits
2aca389
2d12d2f
f357101
7cc4c37
8ec653a
878802e
4dabe0e
564d88d
f4c43e3
a09a07e
6c887e6
0a143f2
0730cd6
246eb57
93f75ea
268db81
a5aa1e8
d480872
6ed1080
120e2bc
2c48272
02127ff
ad13d9e
0ff356c
9f6b5c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -766,8 +766,13 @@ def get_loc(self, key, method=None): | |||||||||||||||
key = Interval(left, right, key.closed) | ||||||||||||||||
else: | ||||||||||||||||
key = self._maybe_cast_slice_bound(key, 'left', None) | ||||||||||||||||
|
||||||||||||||||
start, stop = self._find_non_overlapping_monotonic_bounds(key) | ||||||||||||||||
try: | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You'll also need to do something similar in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it should be the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still need to look into @jorisvandenbossche's comment on raising the error in the engine itself (especially if that's the behaviour for int64), but I think I've addressed everything else. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jorisvandenbossche : There is code in place within the pandas/pandas/_libs/intervaltree.pxi.in Line 104 in 2e38d55
I'm not super well versed in Cython. Is there a graceful way to force this to raise a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the other engines have the key as |
||||||||||||||||
start, stop = self._find_non_overlapping_monotonic_bounds(key) | ||||||||||||||||
except TypeError: | ||||||||||||||||
# get loc should raise KeyError | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typo: |
||||||||||||||||
# if key is hashable but | ||||||||||||||||
# of an incorrect type | ||||||||||||||||
raise KeyError | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we don't use python 3 only constructs yet, the existing is ok. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, I've left the code as is but have improved the reported message as suggested by @rs2 |
||||||||||||||||
|
||||||||||||||||
if start is None or stop is None: | ||||||||||||||||
return slice(start, stop) | ||||||||||||||||
|
@@ -819,7 +824,14 @@ def get_indexer(self, target, method=None, limit=None, tolerance=None): | |||||||||||||||
return np.arange(len(self), dtype='intp') | ||||||||||||||||
|
||||||||||||||||
if self.is_non_overlapping_monotonic: | ||||||||||||||||
start, stop = self._find_non_overlapping_monotonic_bounds(target) | ||||||||||||||||
try: | ||||||||||||||||
start, stop = ( | ||||||||||||||||
self._find_non_overlapping_monotonic_bounds() | ||||||||||||||||
) | ||||||||||||||||
except TypeError: | ||||||||||||||||
# This is probably wrong | ||||||||||||||||
# but not sure what I should do here | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||
return np.array([-1]) | ||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please comment on the choice of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be the same length as |
||||||||||||||||
|
||||||||||||||||
start_plus_one = start + 1 | ||||||||||||||||
if not ((start_plus_one < stop).any()): | ||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -886,6 +886,13 @@ def test_symmetric_difference(self, closed, sort): | |
result = index.symmetric_difference(other, sort=sort) | ||
tm.assert_index_equal(result, expected) | ||
|
||
def test_interval_range_get_indexer_with_different_input_type(self): | ||
samuelsinayoko marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you rename to |
||
# not sure about this one | ||
index = pd.interval_range(0, 1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you parametrize over |
||
# behaviour should be the same as Int64Index and return an | ||
# array with values of -1 | ||
assert np.all(index.get_indexer(['gg']) == np.array([-1])) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use |
||
|
||
@pytest.mark.parametrize('op_name', [ | ||
'union', 'intersection', 'difference', 'symmetric_difference']) | ||
@pytest.mark.parametrize("sort", [None, False]) | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -778,3 +778,9 @@ def test_loc_setitem_empty_append_raises(self): | |||
msg = "cannot copy sequence with size 2 to array axis with dimension 0" | ||||
with pytest.raises(ValueError, match=msg): | ||||
df.loc[0:2, 'x'] = data | ||||
|
||||
def test_loc_getitem_interval_index(self): | ||||
""" GH25087, test get_loc returns key error for interval indexes""" | ||||
idx = pd.interval_range(0, 1.0) | ||||
with pytest.raises(KeyError): | ||||
idx.get_loc('gg') | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of testing here, can you add this as a test case to
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good spot, I must say I wasn't completely clear about the distinction between indexes and indexing with regards to tests. |
Uh oh!
There was an error while loading. Please reload this page.