BUG: Fix interval constructor to handle left=right case #26894

ghost · 2019-06-16T21:29:20Z

closes interval.overlaps() mishandles empty intervals #26893
[] tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Update:

[ ] - Following #26894 (review), needs tests about creating an Intervalindex with '(x,x]', also checking that the repr doesn't blowup.

pep8speaks · 2019-06-16T21:29:22Z

Hello @pilkibun! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-06-17 14:17:31 UTC

codecov · 2019-06-17T03:24:07Z

Codecov Report

Merging #26894 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #26894      +/-   ##
==========================================
- Coverage   91.87%   91.86%   -0.01%     
==========================================
  Files         180      180              
  Lines       50710    50710              
==========================================
- Hits        46588    46584       -4     
- Misses       4122     4126       +4

Flag	Coverage Δ
#multiple	`90.45% <100%> (ø)`	⬆️
#single	`41.12% <0%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/reshape/tile.py	`97.67% <100%> (ø)`	⬆️
pandas/io/gbq.py	`88.88% <0%> (-11.12%)`	⬇️
pandas/core/frame.py	`96.88% <0%> (-0.12%)`	⬇️

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 adc6564...e4e8632. Read the comment docs.

codecov · 2019-06-17T03:24:07Z

Codecov Report

Merging #26894 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #26894      +/-   ##
==========================================
- Coverage   91.87%   91.86%   -0.01%     
==========================================
  Files         180      180              
  Lines       50710    50712       +2     
==========================================
- Hits        46588    46586       -2     
- Misses       4122     4126       +4

Flag	Coverage Δ
#multiple	`90.45% <100%> (ø)`	⬆️
#single	`41.09% <0%> (-0.09%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/reshape/tile.py	`97.67% <100%> (ø)`	⬆️
pandas/io/gbq.py	`88.88% <0%> (-11.12%)`	⬇️
pandas/core/frame.py	`96.88% <0%> (-0.12%)`	⬇️
pandas/core/dtypes/base.py	`100% <0%> (ø)`	⬆️
pandas/core/indexes/base.py	`96.71% <0%> (ø)`	⬆️

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 adc6564...b3d18ba. Read the comment docs.

ghost · 2019-06-17T03:42:01Z

Ready.

jreback · 2019-06-17T11:27:16Z

doc/source/whatsnew/v0.25.0.rst

 Interval
 ^^^^^^^^

 - Construction of :class:`Interval` is restricted to numeric, :class:`Timestamp` and :class:`Timedelta` endpoints (:issue:`23013`)
 - Fixed bug in :class:`Series`/:class:`DataFrame` not displaying ``NaN`` in :class:`IntervalIndex` with missing values (:issue:`25984`)
-
+- :class:Interval will complain when creating point interval with an inconsistent `closed` value (:issue:`26893`)


this is not a very friendly comment, can you re-word, put double backticks around ``closed=`. This is also not clear what you are actually fixing here. What is an inconsistent 'closed' value?

I'm talking about (0,0] or [0,0) when the same point is both open and closed, which makes no sense. Can you suggest a better wording? closed is a kwarg.

Reworded with explicit cases

jreback · 2019-06-17T11:29:23Z

pandas/tests/arrays/interval/test_interval.py

@@ -65,3 +65,23 @@ def test_repr_matches():
    a = repr(idx)
    b = repr(idx.values)
    assert a.replace("Index", "Array") == b
+
+
+def test_point_interval():


make the error conditions as a separate parmeterized test

jreback · 2019-06-17T11:29:31Z

pandas/tests/arrays/interval/test_interval.py

+        with pytest.raises(ValueError, match=match):
+            pd.Interval(0, 0, closed)
+
+    pd.Interval(0, 0, 'neither')  # no exception


assert the result value

assert x==x???

jreback · 2019-06-17T11:29:54Z

pandas/tests/arrays/interval/test_interval.py

+    pd.Interval(0, 0, 'neither')  # no exception
+    pd.Interval(0, 0, 'both')  # no exception
+
+    assert not pd.Interval(0, 1, "left").overlaps(pd.Interval(0, 0, "neither"))


would prefer this is a paramterized test as this is pretty hard to read this way

jreback · 2019-06-17T11:31:48Z

pandas/core/reshape/tile.py

-        i = IntervalIndex([Interval(v, labels[0].right, closed='right')])
+        #
+        # We cannot access labels[0].left because that
+        # calls Interval(left, right, closed='right')


I am not clear what the issue is here, can you show a full example

The comment explains it. labels[0] calls Interval(left, right, closed='right') which throws if left==right. But the object its constructed only to than extract its left and right values, which are available as .left[0] and .right[0]

mroeschke · 2019-06-17T16:33:27Z

doc/source/whatsnew/v0.25.0.rst

 Interval
 ^^^^^^^^

 - Construction of :class:`Interval` is restricted to numeric, :class:`Timestamp` and :class:`Timedelta` endpoints (:issue:`23013`)
 - Fixed bug in :class:`Series`/:class:`DataFrame` not displaying ``NaN`` in :class:`IntervalIndex` with missing values (:issue:`25984`)
-
+- :class:Interval now refuses to create intervals like `[0,0)` or `(0,0]` (:issue:`26893`)


:class:`Interval`

This description is a bit too colloquial. Could you describe the conditions where the new ValueError is raised.

TomAugspurger · 2019-06-17T20:54:39Z

See #26893 (comment). We may not be progressing with this.

jschendel

I'm -1 on this as detailed in #26893 (comment). If my -1 is overruled, then these changes would also need to be propagated to the construction of IntervalArray and IntervalIndex. Currently you can create and operate on arrays/indexes containing an empty interval, only to get an error message when the code actually tries to materialize a scalar interval:

In [1]: import pandas as pd; pd.__version__
Out[1]: '0.25.0.dev0+757.gb3d18ba887'

In [2]: ia = pd.arrays.IntervalArray.from_arrays([1,2], [1, 2])

In [3]: ia.mid
Out[3]: Float64Index([1.0, 2.0], dtype='float64')

In [4]: ia.length
Out[4]: Int64Index([0, 0], dtype='int64')

In [5]: ia[0]
---------------------------------------------------------------------------
ValueError: both/neither sides must be closed when left == right

ghost · 2019-06-18T00:19:57Z

@jschendel, I've seen your comment, lets keep the discussion all in one place in #26893.

Absolutely, this should be fixed. I haven't looked at it yet, but it just might be possible that the exception is raised during specifically repr creation for formatting purposes, in which case this might be solved by silently converting to closed='neither' on creation.

ghost · 2019-06-19T03:39:56Z

closed in favor of #26937

ghost · 2019-06-20T01:40:10Z

While writing the new PR, I came across IntervalTree for the first time. It probably relies on intervals being orderable, right? something to keep in mind if ever reconsidering the issue of a singleton.

datapythonista mentioned this pull request Jun 16, 2019

Task to write build result to GitHub comment microsoft/azure-pipelines-tasks#10665

Closed

pilkibun added 6 commits June 17, 2019 03:28

BUG: pd.Interval accepts ill-defined point interval

023c0ad

DOC: whatsnew

d133fd4

TST: Fix some interval tests

7234906

TST: Fix some more interval tests

df50958

BUG: Fix _format_labels for qcut/qcut, to workaround invalid Interval

96a297a

Style

e4e8632

jreback requested changes Jun 17, 2019

View reviewed changes

jreback requested a review from jschendel June 17, 2019 11:30

jreback added the Interval Interval data type label Jun 17, 2019

jreback reviewed Jun 17, 2019

View reviewed changes

pilkibun added 3 commits June 17, 2019 17:15

Split and parameterize test

b0cd7a3

Revise whatsnew

01ff298

CLN: delete TMI comment

b3d18ba

mroeschke reviewed Jun 17, 2019

View reviewed changes

jschendel suggested changes Jun 17, 2019

View reviewed changes

This was referenced Jun 18, 2019

interval.overlaps() mishandles empty intervals #26893

Open

WIP: overlaps handling of empty intervals in Interval and IntervalArray #26937

Closed

ghost closed this Jun 19, 2019

ghost deleted the fix_point_interval branch June 19, 2019 03:40

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix interval constructor to handle left=right case #26894

BUG: Fix interval constructor to handle left=right case #26894

ghost commented Jun 16, 2019 •

edited by ghost

Loading

pep8speaks commented Jun 16, 2019 •

edited

Loading

codecov bot commented Jun 17, 2019

codecov bot commented Jun 17, 2019 •

edited

Loading

ghost commented Jun 17, 2019

jreback Jun 17, 2019

ghost Jun 17, 2019 •

edited by ghost

Loading

ghost Jun 17, 2019

jreback Jun 17, 2019

ghost Jun 17, 2019

jreback Jun 17, 2019

ghost Jun 17, 2019

jreback Jun 17, 2019

ghost Jun 17, 2019

jreback Jun 17, 2019

ghost Jun 17, 2019

ghost Jun 17, 2019

mroeschke Jun 17, 2019

TomAugspurger commented Jun 17, 2019

jschendel left a comment •

edited

Loading

ghost commented Jun 18, 2019

ghost commented Jun 19, 2019

ghost commented Jun 20, 2019

BUG: Fix interval constructor to handle left=right case #26894

BUG: Fix interval constructor to handle left=right case #26894

Conversation

ghost commented Jun 16, 2019 • edited by ghost Loading

pep8speaks commented Jun 16, 2019 • edited Loading

Comment last updated at 2019-06-17 14:17:31 UTC

codecov bot commented Jun 17, 2019

Codecov Report

codecov bot commented Jun 17, 2019 • edited Loading

Codecov Report

ghost commented Jun 17, 2019

Choose a reason for hiding this comment

ghost Jun 17, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomAugspurger commented Jun 17, 2019

jschendel left a comment • edited Loading

Choose a reason for hiding this comment

ghost commented Jun 18, 2019

ghost commented Jun 19, 2019

ghost commented Jun 20, 2019

ghost commented Jun 16, 2019 •

edited by ghost

Loading

pep8speaks commented Jun 16, 2019 •

edited

Loading

codecov bot commented Jun 17, 2019 •

edited

Loading

ghost Jun 17, 2019 •

edited by ghost

Loading

jschendel left a comment •

edited

Loading