BUG: TimedeltaIndex.intersection #22114

kirkhansen · 2018-07-29T16:25:38Z

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

jreback

pls add a whatsnew note in bug fixes / timedeltas

jreback · 2018-07-30T10:13:51Z

pandas/core/indexes/base.py

@@ -1412,6 +1412,12 @@ def is_monotonic(self):
        """ alias for is_monotonic_increasing (deprecated) """
        return self.is_monotonic_increasing

+    @property


are we actually using this? (it seems you in-lined it below), I actually prefer that to making a new method

The names are similar, but I'm calling this function twice in the intersection function to help me deal with the boolean logic (already hard for me to follow, even with this addition). I can remove this, and put this inline if you really want it that way.

jreback · 2018-07-30T10:14:16Z

pandas/core/indexes/datetimelike.py

+        end = right[-1]
+
+        if end > start:
+            return Index()


why is this not []?

jreback · 2018-07-30T10:14:38Z

pandas/core/indexes/datetimelike.py

+        start = right[0]
+
+        if end < start:
+            return []


worth to consolidate these into 1 routine? as you can call with the ascending arg

jreback · 2018-07-30T10:16:24Z

pandas/core/indexes/datetimelike.py

+                result.offset = frequencies.to_offset(result.inferred_freq)
+            return result
+
+        # handle intersecting things like this


can you add a tiny bit more of expl

jreback · 2018-07-30T10:18:19Z

pandas/tseries/offsets.py

@@ -112,6 +112,41 @@ def wrapper(self, other):
    return wrapper


+def apply_index_wraps(func):


what is going on here?

Been awhile. I don't remember writing this code. I'll guess an artifact of my rebase.

kirkhansen · 2018-09-14T04:47:17Z

Having come back at this, I'm struggling with how to deal with all the edge cases that need to be accounted for before trying to apply this quick method of intersecting, and this is making me question whether or not to even include it.
The quick method cannot allow any 'gaps' between values, unless both indexes have the same gaps, and having to check for that before intersecting seems a lot harder than just using Index.intersection, unless that method is worse than something we can write to check if if values are contiguous based on the frequency passed in or something.

I'm having a hard time describing it clearly, but hopefully this is making sense. If not, I can commit my broken code so you all can see what I'm talking about.

jorisvandenbossche · 2018-09-14T11:57:47Z

A possibility to at least solve the bug would be to fall back to the Index.intersection implementation for the cases where monotonicity does not match?

Basically similar like DatetimeIndex.intersection also falls back to it when eg freq does not match:

pandas/pandas/core/indexes/datetimes.py

Lines 1006 to 1015 in 59cfd8c

    
           elif (other.freq is None or self.freq is None or 
        
                 other.freq != self.freq or 
        
                 not other.freq.isAnchored() or 
        
                 (not self.is_monotonic or not other.is_monotonic)): 
        
               result = Index.intersection(self, other) 
        
               result = self._shallow_copy(result._values, name=result.name, 
        
                                           tz=result.tz, freq=None) 
        
               if result.freq is None: 
        
                   result.freq = to_offset(result.inferred_freq) 
        
               return result

Will not be the most performant option, but at least could fix the bug

kirkhansen · 2018-09-14T13:39:24Z

@jorisvandenbossche I thought I had been doing that, but it seems my check was backwards for that case :/. Correcting that appears to solve the issues. Thanks!

pep8speaks · 2018-09-14T14:03:04Z

Hello @kirkhansen! Thanks for updating the PR.

There are no PEP8 issues in the file pandas/core/indexes/base.py !
There are no PEP8 issues in the file pandas/core/indexes/datetimelike.py !
There are no PEP8 issues in the file pandas/core/indexes/datetimes.py !
There are no PEP8 issues in the file pandas/core/indexes/period.py !
There are no PEP8 issues in the file pandas/core/indexes/timedeltas.py !
There are no PEP8 issues in the file pandas/tests/indexes/datetimes/test_setops.py !
There are no PEP8 issues in the file pandas/tests/indexes/timedeltas/test_setops.py !
There are no PEP8 issues in the file pandas/tests/io/test_pytables.py !
There are no PEP8 issues in the file pandas/tseries/offsets.py !

Comment last updated on December 12, 2018 at 21:29 Hours UTC

codecov · 2018-11-09T05:50:35Z

Codecov Report

Merging #22114 into master will increase coverage by <.01%.
The diff coverage is 86.79%.

@@            Coverage Diff             @@
##           master   #22114      +/-   ##
==========================================
+ Coverage   92.21%   92.22%   +<.01%     
==========================================
  Files         162      162              
  Lines       51768    51761       -7     
==========================================
- Hits        47739    47734       -5     
+ Misses       4029     4027       -2

Flag	Coverage Δ
#multiple	`90.62% <86.79%> (ø)`	⬆️
#single	`43.14% <47.16%> (+0.14%)`	⬆️

Impacted Files	Coverage Δ
pandas/core/indexes/datetimes.py	`96.8% <ø> (+0.46%)`	⬆️
pandas/core/indexes/period.py	`93.06% <100%> (ø)`	⬆️
pandas/tseries/offsets.py	`96.86% <100%> (+0.01%)`	⬆️
pandas/core/indexes/base.py	`96.27% <100%> (ø)`	⬆️
pandas/core/indexes/timedeltas.py	`90.69% <33.33%> (+0.26%)`	⬆️
pandas/core/indexes/datetimelike.py	`96.24% <87.5%> (-1.06%)`	⬇️

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 9bc42f9...4aac7e8. Read the comment docs.

jreback · 2018-12-09T20:08:10Z

if you can merge master

Fixes pandas-dev#17391

shallow_copy appears to no longer accept tz...

let's see what's broken now.

mroeschke · 2018-12-26T07:16:20Z

You'll need to run isort on this file:

2018-12-12T21:36:13.4930067Z ERROR: /home/vsts/work/1/s/pandas/core/indexes/datetimelike.py Imports are incorrectly sorted.

mroeschke · 2018-12-26T07:17:02Z

pandas/tseries/offsets.py

+    first = getattr(first, 'freq', None)
+    second = getattr(second, 'freq', None)
+    are_offsets_equal = True
+    if first is None or second is None or first != second:


Just return this statement; no need for the intermediate variable

mroeschke · 2018-12-26T07:17:44Z

pandas/tests/io/test_pytables.py

@@ -4116,6 +4116,7 @@ def test_append_to_multiple_dropna(self):
        df2 = tm.makeTimeDataFrame().rename(columns=lambda x: "%s_2" % x)
        df1.iloc[1, df1.columns.get_indexer(['A', 'B'])] = np.nan
        df = concat([df1, df2], axis=1)
+        print(df)


This print should be removed.

mroeschke · 2018-12-26T07:19:51Z

pandas/core/indexes/datetimelike.py

+        """
+        Speedy intersection that works only if certain assumptions are met.
+        See intersection for details.
+        Parameters


Could you give a bit more color to Parameters and Returns

WillAyd · 2019-02-27T23:30:36Z

Closing as stale ping if you'd like to continue

kirkhansen changed the title ~~BUG: TimedeltaIndex.intersection #17433~~ BUG: TimedeltaIndex.intersection Jul 29, 2018

kirkhansen force-pushed the fix-descending-timedeltaindex-intersect branch from 62d9e3e to 5af3c53 Compare July 29, 2018 17:08

jreback requested changes Jul 30, 2018

View reviewed changes

jreback added Bug Reshaping Concat, Merge/Join, Stack/Unstack, Explode Timedelta Timedelta data type labels Jul 30, 2018

kirkhansen force-pushed the fix-descending-timedeltaindex-intersect branch from 5af3c53 to 808ce4e Compare September 14, 2018 14:02

kirkhansen force-pushed the fix-descending-timedeltaindex-intersect branch from 808ce4e to 5703681 Compare November 9, 2018 05:50

kirkhansen and others added 5 commits December 12, 2018 13:49

BUG: TimedeltaIndex.intersection

22a94f4

Fixes pandas-dev#17391

What's new entry

568cb5b

Remove redeclared functions

7e0079f

Attempting to fix some stuff

a051c0b

shallow_copy appears to no longer accept tz...

Previously failed tests paassing

2f5513c

let's see what's broken now.

kirkhansen force-pushed the fix-descending-timedeltaindex-intersect branch from 16965c5 to 2f5513c Compare December 12, 2018 19:57

Be less confusing about dealing with the frequency

4aac7e8

mroeschke reviewed Dec 26, 2018

View reviewed changes

WillAyd closed this Feb 27, 2019

makbigc mentioned this pull request Mar 11, 2019

[BUG] TimedeltaIndex intersects with decreasing TimedeltaIndex #25661

Closed

		@@ -112,6 +112,41 @@ def wrapper(self, other):
		return wrapper


		def apply_index_wraps(func):

Uh oh!

BUG: TimedeltaIndex.intersection #22114

BUG: TimedeltaIndex.intersection #22114

Uh oh!

Conversation

kirkhansen commented Jul 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jreback left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirkhansen Sep 14, 2018 • 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 14, 2018

Uh oh!

jorisvandenbossche commented Sep 14, 2018

Uh oh!

kirkhansen commented Sep 14, 2018

Uh oh!

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

Uh oh!

Comment last updated on December 12, 2018 at 21:29 Hours UTC

Uh oh!

codecov bot commented Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jreback commented Dec 9, 2018

Uh oh!

mroeschke commented Dec 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mroeschke Dec 26, 2018 • 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!

WillAyd commented Feb 27, 2019

Uh oh!

Uh oh!

kirkhansen commented Jul 29, 2018 •

edited

Loading

kirkhansen Sep 14, 2018 •

edited

Loading

pep8speaks commented Sep 14, 2018 •

edited

Loading

codecov bot commented Nov 9, 2018 •

edited

Loading

mroeschke Dec 26, 2018 •

edited

Loading