-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: IntervalIndex is_non_overlapping_monotonic is incorrect when endpoints are closed on both sides #16588
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
…' returned incorrect values
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.
some changes. add a whatsnew note. 0.21.0 conversion bug fixes
pandas/core/indexes/interval.py
Outdated
if self.closed == 'both': | ||
return ((self.right[:-1] < self.left[1:]).all() or | ||
(self.left[:-1] > self.right[1:]).all()) | ||
else: |
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.
don't use the else here
add a comment on these cases (left/right).
neighter case? (short circuit to False)
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.
Working on this now. Why would the neither case short circuit to False? Can't we have intervals [(0, 1), (1, 2)] where it is non-overlapping and also cases where it is overlapping [(0, 1), (.5, 3)]
Thanks!
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 is checking monotonicity as well. so you can do the same for neither.
def test_is_non_overlapping_monotonic(self): | ||
index = IntervalIndex.from_tuples( | ||
[(0, 1), (2, 3), (4, 5)], closed='both') | ||
assert index.is_non_overlapping_monotonic == True |
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.
use is True
assert index.is_non_overlapping_monotonic == True | ||
|
||
index = IntervalIndex.from_breaks(range(4), closed='both') | ||
assert index.is_non_overlapping_monotonic == False |
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.
use is False
… with change information.
Codecov Report
@@ Coverage Diff @@
## master #16588 +/- ##
==========================================
+ Coverage 90.75% 90.75% +<.01%
==========================================
Files 161 161
Lines 51097 51099 +2
==========================================
+ Hits 46372 46374 +2
Misses 4725 4725
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #16588 +/- ##
==========================================
+ Coverage 90.75% 90.75% +<.01%
==========================================
Files 161 161
Lines 51097 51099 +2
==========================================
+ Hits 46372 46374 +2
Misses 4725 4725
Continue to review full report at Codecov.
|
Fixed the issues from the review, except for changing Added additional tests and updated the whatsnew section in the docs. Thanks. |
@jschendel : Confirmed and closing. |
git diff upstream/master --name-only -- '*.py' | flake8 --diff