BUG: TimedeltaIndex.intersection #17433

kirkhansen · 2017-09-05T02:53:32Z

closes BUG: TimedeltaIndex.intersection fails for decreasing TimedeltaIndex #17391
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

codecov · 2017-09-05T03:26:03Z

Codecov Report

Merging #17433 into master will decrease coverage by 0.02%.
The diff coverage is 84.37%.

@@            Coverage Diff             @@
##           master   #17433      +/-   ##
==========================================
- Coverage   91.16%   91.13%   -0.03%     
==========================================
  Files         163      163              
  Lines       49581    49603      +22     
==========================================
+ Hits        45199    45208       +9     
- Misses       4382     4395      +13

Flag	Coverage Δ
#multiple	`88.92% <84.37%> (-0.01%)`	⬇️
#single	`40.24% <12.5%> (-0.08%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/timedeltas.py	`90.22% <84.37%> (-0.37%)`	⬇️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.72% <0%> (-0.1%)`	⬇️

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 c2d0481...98bbc51. Read the comment docs.

codecov · 2017-09-05T03:26:14Z

Codecov Report

Merging #17433 into master will decrease coverage by 0.05%.
The diff coverage is 80.88%.

@@            Coverage Diff             @@
##           master   #17433      +/-   ##
==========================================
- Coverage   91.18%   91.13%   -0.06%     
==========================================
  Files         163      163              
  Lines       49545    49555      +10     
==========================================
- Hits        45179    45163      -16     
- Misses       4366     4392      +26

Flag	Coverage Δ
#multiple	`88.92% <80.88%> (-0.04%)`	⬇️
#single	`40.26% <55.88%> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/timedeltas.py	`91.17% <ø> (+0.58%)`	⬆️
pandas/core/indexes/datetimes.py	`95.68% <ø> (+0.15%)`	⬆️
pandas/core/indexes/base.py	`96.28% <100%> (ø)`	⬆️
pandas/core/indexes/period.py	`92.74% <100%> (ø)`	⬆️
pandas/core/indexes/datetimelike.py	`94.75% <80%> (-2.14%)`	⬇️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/plotting/_converter.py	`63.23% <0%> (-1.82%)`	⬇️
pandas/core/frame.py	`97.77% <0%> (-0.1%)`	⬇️
pandas/core/categorical.py	`95.51% <0%> (-0.02%)`	⬇️
pandas/core/base.py	`96.01% <0%> (ø)`	⬆️
... and 6 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 fa557f7...918276c. Read the comment docs.

sinhrks · 2017-09-05T05:04:45Z

Thanks for the PR. It should have the similar logic as DatetimeIndex (may better to move to datetimelike.py).
https://github.com/pandas-dev/pandas/blob/master/pandas/core/indexes/datetimes.py#L1217

Also, PeriodIndex doesn't have the efficient logic ATM, but should be the same.

sinhrks · 2017-09-05T05:05:41Z

pandas/tests/indexes/timedeltas/test_setops.py

+def test_intersection_bug_17391():
+    idx1 = pd.to_timedelta(range(3), unit='s')
+    idx2 = pd.to_timedelta(range(2, -1, -1), unit='s')
+    print(idx1)


please remove.

sinhrks · 2017-09-05T05:06:27Z

pandas/tests/indexes/timedeltas/test_setops.py

+
+
+def test_intersection_bug_17391():
+    idx1 = pd.to_timedelta(range(3), unit='s')


can u also add tests with not-sorted index?

jreback · 2017-09-05T10:13:34Z

pandas/core/indexes/timedeltas.py

@@ -443,6 +443,15 @@ def f(x):
            result = result.astype('int64')
        return result

+    @property


just use is_monotonic_ascending

Looks like you may want the (private) _is_strictly_monotonic_increasing property.

jreback · 2017-09-05T10:14:09Z

pandas/core/indexes/timedeltas.py

@@ -596,6 +605,38 @@ def _wrap_union_result(self, other, result):
        name = self.name if self.name == other.name else None
        return self._simple_new(result, name=name, freq=None)

+    def _slice_by_value(self, start, end):


all of these are just used once. pls put them in-line to the function.

jreback · 2017-09-05T10:14:26Z

pandas/core/indexes/timedeltas.py

@@ -607,7 +648,8 @@ def intersection(self, other):

        Returns
        -------
-        y : Index or TimedeltaIndex
+        Index or TimedeltaIndex
+            A shallow copied intersection between the two things passed in


jreback · 2017-09-05T10:14:52Z

pandas/tests/indexes/timedeltas/test_setops.py

+
+
+def test_intersection_bug_17391():
+    idx1 = pd.to_timedelta(range(3), unit='s')


put the gh issue number as a comment.

jreback · 2017-09-05T10:17:33Z

pandas/tests/indexes/timedeltas/test_setops.py

@@ -74,3 +74,66 @@ def test_intersection_bug_1708(self):
        result = index_1 & index_2
        expected = timedelta_range('1 day 01:00:00', periods=3, freq='h')
        tm.assert_index_equal(result, expected)
+
+
+def test_intersection_bug_17391():


pls just name this test_intersection; this bug* scheme is old and not used anymore (you can fix the other one as well). the 'main' intersection tests should be in a test named test_intersection.

jreback · 2017-09-05T10:18:04Z

pandas/tests/indexes/timedeltas/test_setops.py

+    idx2 = pd.to_timedelta(range(2, -1, -1), unit='s')
+    print(idx1)
+    print(idx2)
+    assert len(idx1.intersection(idx2)) == 3


don't compare like this. construct the expected index as ordering is important here. use tm.assert_index_equal

jreback · 2017-09-05T10:18:44Z

pandas/tests/indexes/timedeltas/test_setops.py

+    idx1 = pd.to_timedelta(range(2, 6), unit='s')
+    idx2 = pd.to_timedelta(range(3), unit='s')
+    intersection = idx1.intersection(idx2)
+    assert intersection.equals(TimedeltaIndex(['00:00:02']))


use a pattern of

result = .... expected = ... tm.assert_index_equals(result, expected)

jreback · 2017-09-05T10:18:58Z

pandas/tests/indexes/timedeltas/test_setops.py

+    idx1 = pd.to_timedelta(range(6, 4, -1), unit='s')
+    idx2 = pd.to_timedelta(range(4, 1, -1), unit='s')
+    intersection = idx1.intersection(idx2)
+    print(idx1)


remove all prints

pep8speaks · 2017-09-13T00:49:47Z

Hello @kirkhansen! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on September 14, 2017 at 02:39 Hours UTC

jreback · 2017-09-13T00:58:57Z

pandas/core/indexes/base.py

@@ -1201,6 +1201,12 @@ def is_monotonic(self):
        return self.is_monotonic_increasing

    @property
+    def is_strictly_monotonic(self):


make this private (lead with the underscore)

Some background: when we added _is_strictly_monotonic_increasing, we went back and forth on whether or not to make it public. For now we're making it internal only until people actually ask for it.

jreback · 2017-09-13T01:03:12Z

pandas/core/indexes/datetimelike.py

+            return type(self)(data=[], **empty_params)
+        return left._slice_by_value(start, end)
+
+    def _offsets_equal(self, other):


this shouldn't be here and is pretty clunky, if you can't find something useful in pandas.tseries.offsets, then make a function there. but is there a reason you cannot do

getattr(self, 'offset, None) != getattr(other, 'offset', None)?

you would have to demonstrate how the .isAnchored actually matters here

Looks like 3 tests fail when doing just getattr(self, 'offset', None) != getattr(self, 'offset', None) for slightly different reasons.

pandas/tests/indexes/datetimes/test_setops.py:87 TestDatetimeIndex.test_intersection : result.freq != expected.freq

pandas/tests/indexes/datetimes/test_setops.py:151 TestDatetimeIndex.test_intersection_bug_1708: assert 3 == 0

pandas/tests/indexes/datetimes/test_setops.py:283 TestBusinessDatetimeIndex.test_intersection: assert None == <Minute>

The tests that run locally seem to be fine with isAnchored being gone.

jreback · 2017-09-13T01:04:08Z

pandas/core/indexes/datetimelike.py

+                result.offset = frequencies.to_offset(result.inferred_freq)
+            return result
+
+        lenghts = len(self), len(other)


jreback · 2017-09-13T01:04:36Z

pandas/core/indexes/datetimelike.py

+        # handle intersecting things like this
+        # idx1 = pd.to_timedelta((1, 2, 3, 4, 5, 6, 7, 8), unit='s')
+        # idx2 = pd.to_timedelta((2, 3, 4, 8), unit='s')
+        if lenghts[0] != lenghts[1] and (


why do you need this special case?

The way that fast indexing works will produce a wrong result when running an intersection with those two examples. The result will be (2, 3, 4, 5, 6, 7, 8) instead of (2, 3, 4, 8).

Interestingly enough, when this case is gone, the reason I added the assert kind in ['ix', 'loc', 'getitem', None] shows up as an error in one of the tests.

pandas/tests/indexes/period/test_setops.py:133 TestPeriodIndex.test_intersection: AssertionError

pandas/tests/indexes/period/test_setops.py:155 TestPeriodIndex.test_intersection_cases: AssertionError

I added None as None is in both the base _maybe_cast_slice_bound and the timedeltas _maybe_cast_slice_bound.

jreback · 2017-09-13T01:04:51Z

pandas/core/indexes/datetimelike.py

+        if self_ascending != other.is_monotonic_increasing:
+            other = other.sort_values(ascending=self_ascending)
+
+        # Thanks, PeriodIndex


This can go away now with the changes to intersect_ascending and intersect_descending not needing to create an empty index of type(self).

jreback · 2017-09-13T01:06:13Z

pandas/core/indexes/datetimelike.py

+        end = right[-1]
+
+        if end > start:
+            return type(self)(data=[], **empty_params)


this is very clunky. again just return an indexer (or values are ok), and wrap at a higher level, then you don't need all of these params.

jreback · 2017-09-13T01:06:25Z

pandas/core/indexes/datetimelike.py

+        else:
+            intersected = self._intersect_descending(other, **empty_params)
+
+        name = self.name


_get_consensus_name

jreback · 2017-09-13T01:06:42Z

pandas/core/indexes/period.py

@@ -843,7 +843,7 @@ def _maybe_cast_slice_bound(self, label, side, kind):
        Value of `side` parameter should be validated in caller.

        """
-        assert kind in ['ix', 'loc', 'getitem']
+        assert kind in ['ix', 'loc', 'getitem', None]


jreback · 2017-09-13T01:07:04Z

pandas/tests/indexes/timedeltas/test_setops.py

@@ -74,3 +75,99 @@ def test_intersection_bug_1708(self):
        result = index_1 & index_2
        expected = timedelta_range('1 day 01:00:00', periods=3, freq='h')
        tm.assert_index_equal(result, expected)
+
+
+def test_intersection_intersects_ascending():


parametrize this

jreback · 2017-09-13T01:07:33Z

pandas/tests/indexes/timedeltas/test_setops.py

+    assert result.equals(TimedeltaIndex(['00:00:02']))
+
+
+def test_intersection_intersects_descending():


parametrize

kirkhansen · 2017-09-13T01:49:28Z

@jreback most of the logic here is from a merge between the old timedeltas.py intersection, and the datetimes.py intersection. I assumed the logic in both of these were there for good reason. I'm fine with cleaning those types of things up per your merge request, but just know it was mostly copy paste, and I took the most restrictive version of each piece between the intersection functions I could find without fully understanding their reason for being there.

jreback · 2017-09-13T02:01:39Z

@jreback most of the logic here is from a merge between the old timedeltas.py intersection, and the datetimes.py intersection. I assumed the logic in both of these were there for good reason. I'm fine with cleaning those types of things up per your merge request, but just know it was mostly copy paste, and I took the most restrictive version of each piece between the intersection functions I could find without fully understanding their reason for being there.

@kirkhansen yep I see that. When we refactor always like to clean up what is possible. For sure nice job on combining the disparate sub-class routines.

Fixes pandas-dev#17391

TomAugspurger · 2017-09-24T14:09:45Z

@kirkhansen some tests failures in pandas/tests/io/test_pytables.py::TestHDFStore, that I think are relevant Can you take a look (doing a release at the end of the week).

kirkhansen · 2017-09-25T19:41:43Z

some tests failures in pandas/tests/io/test_pytables.py::TestHDFStore, that I think are relevant Can you take a look (doing a release at the end of the week).

@TomAugspurger I haven't had much time for coding as of late. I haven't been able to reproduce this error locally (all the tests pass with my versions of things), but I'll try and dig into this tonight or tomorrow.

TomAugspurger · 2017-09-25T19:52:17Z

K. I just pulled your branch and wasn't able to reproduce either. I should have some time tomorrow to debug if you're busy.

kirkhansen · 2017-09-26T19:47:06Z

@TomAugspurger I hate to say it, but if you're attempting to release at the end of the week, you may want to pick this up. I was unable to spend time on it yesterday and doubt I'll be able to look at this tonight.

jreback · 2017-11-10T20:21:01Z

can you rebase / update

jreback · 2017-12-10T23:32:17Z

closing as stale, but if you'd like to keep working, ping and we can reopen

kirkhansen · 2018-07-20T01:52:02Z

@jreback I've been poking around at this again, getting stuff back up to date in hopes of resolving the build failure.

kirkhansen · 2018-07-24T12:27:20Z

FWIW, the pytables tests pass now, but i get errors with LooseVersion. Those same errors show up in the master branch of my builds, so I'm not sure what its problem is since it's a copy of your master, and your master is passing.

TomAugspurger · 2018-07-24T13:27:12Z

FYI @kirkhansen you'll need to open a new PR. Github doesn't support closing a PR, bushing to the branch, and then reopening.

sinhrks added Bug Timedelta Timedelta data type labels Sep 5, 2017

sinhrks reviewed Sep 5, 2017

View reviewed changes

jreback requested changes Sep 5, 2017

View reviewed changes

kirkhansen force-pushed the fix-descending-timedeltaindex-intersect branch 5 times, most recently from fd3434a to 1eb8918 Compare September 13, 2017 00:49

jreback requested changes Sep 13, 2017

View reviewed changes

BUG: TimedeltaIndex.intersection

918276c

Fixes pandas-dev#17391

kirkhansen force-pushed the fix-descending-timedeltaindex-intersect branch from 02c4bc6 to 918276c Compare September 14, 2017 02:39

jreback closed this Dec 10, 2017

kirkhansen mentioned this pull request Jul 29, 2018

BUG: TimedeltaIndex.intersection #22114

Closed

4 tasks



		def test_intersection_bug_17391():
		idx1 = pd.to_timedelta(range(3), unit='s')

		assert result.equals(TimedeltaIndex(['00:00:02']))


		def test_intersection_intersects_descending():

Uh oh!

BUG: TimedeltaIndex.intersection #17433

BUG: TimedeltaIndex.intersection #17433

Uh oh!

Conversation

kirkhansen commented Sep 5, 2017

Uh oh!

codecov bot commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codecov bot commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sinhrks commented Sep 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pep8speaks commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated on September 14, 2017 at 02:39 Hours UTC

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirkhansen Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirkhansen Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirkhansen commented Sep 13, 2017

Uh oh!

jreback commented Sep 13, 2017

codecov bot commented Sep 5, 2017 •

edited

Loading

codecov bot commented Sep 5, 2017 •

edited

Loading

pep8speaks commented Sep 13, 2017 •

edited

Loading

kirkhansen Sep 13, 2017 •

edited

Loading

kirkhansen Sep 13, 2017 •

edited

Loading