-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: Fix IntervalIndex.get_loc/get_indexer for IntervalIndex of length one #20946
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
Conversation
ef1aa4d
to
52d8e3b
Compare
Codecov Report
@@ Coverage Diff @@
## master #20946 +/- ##
==========================================
- Coverage 91.81% 91.81% -0.01%
==========================================
Files 153 153
Lines 49479 49478 -1
==========================================
- Hits 45428 45427 -1
Misses 4051 4051
Continue to review full report at Codecov.
|
doc/source/whatsnew/v0.23.0.txt
Outdated
@@ -1245,6 +1245,7 @@ Indexing | |||
- Bug in ``Series.is_unique`` where extraneous output in stderr is shown if Series contains objects with ``__ne__`` defined (:issue:`20661`) | |||
- Bug in ``.loc`` assignment with a single-element list-like incorrectly assigns as a list (:issue:`19474`) | |||
- Bug in partial string indexing on a ``Series/DataFrame`` with a monotonic decreasing ``DatetimeIndex`` (:issue:`19362`) | |||
- Bug in ``IntervalIndex.get_loc()`` and ``IntervalIndex.get_indexer()`` when used with an :class:`IntervalIndex` containing a single interval (:issue:`17284`, :issue:`20921`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these i the api.rst? if not can you can add and put refs here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -544,6 +552,16 @@ def test_get_indexer_subintervals(self): | |||
expected = np.array([0, 0, 0], dtype='intp') | |||
tm.assert_numpy_array_equal(actual, expected) | |||
|
|||
# Make consistent with test_interval_new.py (see #16316, #16386) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is consistent with those tests, you can remove from there I think (not sure exactly how we are tracking 'new' things vs current)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't consistent with the new tests, as the new behavior should only allow for exact matches when querying with Interval
objects. Mostly added this a a note to whoever implements the new behavior (probably me).
I've taken a stab at implementing the new behavior in the past, but it looks like it will require a pretty large change, as changing one method to implement the new behavior often causes another (also with new proposed behavior) to break, so it seems like it'd at least approach an all or nothing type of change. Hopefully will carve out some time to do it fully somewhat soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k that is fine. yeah converting to use the new tests is a major effort :>
contains | ||
from_tuples | ||
get_indexer | ||
get_loc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reorganized here to be alphabetical.
thanks @jschendel. always nice to have a great patch from you! |
git diff upstream/master -u -- "*.py" | flake8 --diff