err#8038: Index.shift() gives confusing error message when no Datetim… #11211

hamedhsn · 2015-09-30T22:59:02Z

…eIndex

max-sixty · 2015-09-30T23:30:12Z

Is this the behavior you want though, vs. a clearer error message? This doesn't seem like it's shifting the index; it seems like it's adding 1...

        result = pd.Index(range(0,10,2)).shift(1)
        expected = pd.Index([1, 3, 5, 7, 9], dtype='int64')

jreback · 2015-10-01T10:55:14Z

@hamedhsn I don't this should even work on a non-datetimelike index as it doesn't make any sense. So we should prob raise a TypeError.

jreback · 2015-10-05T11:01:31Z

pandas/core/index.py

-        offset = periods * freq
+        try:
+            offset = periods * freq
+        except TypeError:


no, pls. raise a NotImplementedError for all ops in core/index.py for .shift. Then define this in tseries/base/DatetimeIndexOpsMixIn

jreback · 2015-10-05T11:01:46Z

pls add a test and a whatsnew note in 0.17.1

hamedhsn · 2015-10-06T21:58:02Z

@jreback
1)why do I need to change tseries/base/DatetimeIndexOpsMixIn ?
2)where should I add tests?

jreback · 2015-10-06T22:49:44Z

I indicated above. The .shfit incore/index.pyapplies toIndex/Int64Indexetc. These are not valid for shifting and so should raiseNotImplementedError. Then make the actual change intseries/base/DatetimeIndexOpsMixInwhich is the base class for the datetimelikes (`DatetimeIndex,TimeDeltaIndex,PeriodIndex``) where shifting makes sense

hamedhsn · 2015-10-06T23:24:57Z

sure, but there is no issue with DatetimeIndex using shift() with or without freq. that was my question.

the error happens for Index/Int64Index which i changed it to raise NotImplementedError.
btw where do i have to add the related tests?

jreback · 2015-10-07T12:09:22Z

@hamedhsn you can add tests in test_index (though the timeseries ones are prob in tseries/test_base)

hamedhsn · 2015-10-15T11:30:22Z

@jreback this also should be fine

max-sixty · 2015-10-15T18:27:15Z

@jreback @hamedhsn I think this behavior is confusing. I think there are some reasons to shift a non-DateTime-like index, but it shouldn't be done with a freq.

Do you guys think this is clear and expected behavior?

        idx = Index(range(5))   
        result = idx.shift(periods=2, freq=2)
        expected = pd.Index(range(4, 9))
        self.assert_index_equal(result, expected)

A version I could see being helpful is to shift by a number of items. i.e. this:

        idx = Index(range(5,10))   
        result = idx.shift(1)
        expected = pd.Index(range(6, 11))
        self.assert_index_equal(result, expected)

...although I'm more against the current implementation than I am fighting for this specific implementation.

As ever, open to being wrong though.

jreback · 2015-10-15T22:28:26Z

@MaximilianR that would be a major change to add a freq to a non-timeseries Index. So wouldn't want to support that (here).

max-sixty · 2015-10-15T22:44:05Z

@jreback That's exactly what I'm saying. This PR enables freq for shifting non-timeseries index (or am I woefully mistaken)? This is a copy paste from the new test code:

        idx = Index(range(5))   
        result = idx.shift(periods=2, freq=2)
        expected = pd.Index(range(4, 9))
        self.assert_index_equal(result, expected)

jreback · 2015-10-15T23:00:24Z

that's out of scope for this PR
would be unexpected as freq is not a property of an Index

you are free to propose it but make an issue

max-sixty · 2015-10-15T23:09:13Z

I don't think I'm being clear. Your comment here would fix the concern, but that hasn't been added.
Or I'm likely missing something, so I'll leave it for now

jreback · 2015-10-18T13:57:35Z

doc/source/whatsnew/v0.17.1.txt

@@ -26,7 +26,7 @@ Enhancements

 Other Enhancements
 ^^^^^^^^^^^^^^^^^^
-
+- raising ``NotImplementedError`` for ``Index.shift()`` when no ``DatetimeIndex`` is used.


say: raise NotImplementedError in Index.shift for non-supported index types. (and include the issue number)

hamedhsn · 2015-10-18T21:42:25Z

@jreback is this implementation correct? let me know if I need to anything

jreback · 2015-10-18T23:21:17Z

doc/source/whatsnew/v0.17.1.txt

@@ -27,10 +27,14 @@ Enhancements

 Other Enhancements
 ^^^^^^^^^^^^^^^^^^
+<<<<<<< HEAD
+- raising ``NotImplementedError`` for ``Index.shift()`` when no ``DatetimeIndex`` is used.


rebase artifcacts

add the issue number

put in API changes as this 'worked' before

jreback · 2015-10-18T23:23:20Z

a lot closer....still a couple of comments

hamedhsn · 2015-10-19T22:41:48Z

@jreback applied your comments

hamedhsn · 2015-11-10T19:45:00Z

@jreback applied the comments

jreback · 2015-11-11T12:50:03Z

pandas/tseries/tests/test_base.py

@@ -45,6 +45,14 @@ def test_ops_properties_basic(self):
        self.assertEqual(s.day,10)
        self.assertRaises(AttributeError, lambda : s.weekday)

+    def test_shift_index(self):


move this to test_index.py inside that Base class

further you will need to create the index with self.create_index()

hamedhsn · 2015-11-11T22:20:19Z

@jreback should be fine now hopefully

jreback · 2015-11-15T18:42:20Z

@hamedhsn if you would like to update according to comments would be great

hamedhsn · 2015-11-15T19:08:41Z

@jreback I think I did, used self.create_index(), put that in DateTimelike. and for the other one moved that to based class in test_index.py and used self.create_index(). and rebased that.

jreback · 2015-11-16T12:24:21Z

pandas/tests/test_index.py

+        # test shift for datetimeIndex and non datetimeIndex
+        # err8083
+
+        drange = self.create_index()


this is ONLY true for DatetimeIndex. Need separate expectation for TimedeltaIndex/PeriodIndex (IOW separate methods).

hamedhsn · 2015-11-17T19:29:37Z

@jreback added the other two tests under their own test classes

jreback · 2015-11-18T12:14:27Z

xref #11452

jreback · 2015-11-18T12:15:31Z

pandas/tests/test_index.py

+        idx = self.create_index()
+        self.assertRaises(NotImplementedError, idx.shift, 1)
+
+        result = idx.shift(periods=2, freq=2)


take out this call right here as it will fail by definition (leave the assertRaises)

jreback · 2015-11-18T12:16:57Z

@hamedhsn ok, tests look good (you just need to remove an error line and they would pass). pls add the other requested tests and good 2 go.

err#8038 : add fix, note and test err#8038 : add fix, note and test fix added with tests modification Index.shift() gives confusing error message when no DatetimeIndex test, fix note change tests add issue number test change test change add tests add test

hamedhsn · 2015-11-18T19:51:02Z

@jreback applied your comments.rebased it

jreback · 2015-11-18T20:12:21Z

merged via a05eceb

thanks!

melroy89 · 2017-01-18T13:36:08Z

It seems I still have this issue see my comment in: #11452. I'm using pandas v0.19.2 via pip.

jreback added the Error Reporting Incorrect or improved errors from pandas label Oct 1, 2015

jreback reviewed Oct 5, 2015
View reviewed changes

hamedhsn force-pushed the err#8038 branch 4 times, most recently from f14446d to 0393571 Compare October 14, 2015 21:40

jreback reviewed Oct 18, 2015
View reviewed changes

jreback added this to the 0.17.1 milestone Oct 18, 2015

hamedhsn force-pushed the err#8038 branch from c12de35 to 558aa18 Compare November 10, 2015 19:43

jreback reviewed Nov 11, 2015
View reviewed changes

hamedhsn force-pushed the err#8038 branch 2 times, most recently from c92864e to cb1d357 Compare November 11, 2015 22:19

jreback modified the milestones: Next Major Release, 0.17.1 Nov 15, 2015

jreback reviewed Nov 16, 2015
View reviewed changes

hamedhsn force-pushed the err#8038 branch from cb1d357 to 5a9dd0c Compare November 17, 2015 19:22

max-sixty mentioned this pull request Nov 17, 2015

CLN: tshift & shift could be consolidated #11631

Closed

jreback reviewed Nov 18, 2015
View reviewed changes

jreback added this to the 0.17.1 milestone Nov 18, 2015

hamedhsn force-pushed the err#8038 branch from 5a9dd0c to 58769ec Compare November 18, 2015 19:48

jreback closed this Nov 18, 2015

jreback mentioned this pull request Nov 18, 2015

ERR: Index.shift() gives confusing error message when no DatetimeIndex #8038

Closed

naught101 mentioned this pull request Nov 30, 2015

BUG: Frequency-based shifting doesn't work with groupby and MultiIndex #11452

Closed

sinhrks mentioned this pull request Apr 29, 2016

Calling shift on a DatetimeIndex of length 0 returns an Index instead of a DatetimeIndex #9903

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

err#8038: Index.shift() gives confusing error message when no Datetim… #11211

err#8038: Index.shift() gives confusing error message when no Datetim… #11211

hamedhsn commented Sep 30, 2015

max-sixty commented Sep 30, 2015

jreback commented Oct 1, 2015

jreback Oct 5, 2015

jreback commented Oct 5, 2015

hamedhsn commented Oct 6, 2015

jreback commented Oct 6, 2015

hamedhsn commented Oct 6, 2015

jreback commented Oct 7, 2015

hamedhsn commented Oct 15, 2015

max-sixty commented Oct 15, 2015

jreback commented Oct 15, 2015

max-sixty commented Oct 15, 2015

jreback commented Oct 15, 2015

max-sixty commented Oct 15, 2015

jreback Oct 18, 2015

hamedhsn commented Oct 18, 2015

jreback Oct 18, 2015

jreback Oct 18, 2015

jreback Oct 18, 2015

jreback commented Oct 18, 2015

hamedhsn commented Oct 19, 2015

hamedhsn commented Nov 10, 2015

jreback Nov 11, 2015

hamedhsn commented Nov 11, 2015

jreback commented Nov 15, 2015

hamedhsn commented Nov 15, 2015

jreback Nov 16, 2015

hamedhsn commented Nov 17, 2015

jreback commented Nov 18, 2015

jreback Nov 18, 2015

jreback commented Nov 18, 2015

hamedhsn commented Nov 18, 2015

jreback commented Nov 18, 2015

melroy89 commented Jan 18, 2017

err#8038: Index.shift() gives confusing error message when no Datetim… #11211

err#8038: Index.shift() gives confusing error message when no Datetim… #11211

Conversation

hamedhsn commented Sep 30, 2015

max-sixty commented Sep 30, 2015

jreback commented Oct 1, 2015

Choose a reason for hiding this comment

jreback commented Oct 5, 2015

hamedhsn commented Oct 6, 2015

jreback commented Oct 6, 2015

hamedhsn commented Oct 6, 2015

jreback commented Oct 7, 2015

hamedhsn commented Oct 15, 2015

max-sixty commented Oct 15, 2015

jreback commented Oct 15, 2015

max-sixty commented Oct 15, 2015

jreback commented Oct 15, 2015

max-sixty commented Oct 15, 2015

Choose a reason for hiding this comment

hamedhsn commented Oct 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Oct 18, 2015

hamedhsn commented Oct 19, 2015

hamedhsn commented Nov 10, 2015

Choose a reason for hiding this comment

hamedhsn commented Nov 11, 2015

jreback commented Nov 15, 2015

hamedhsn commented Nov 15, 2015

Choose a reason for hiding this comment

hamedhsn commented Nov 17, 2015

jreback commented Nov 18, 2015

Choose a reason for hiding this comment

jreback commented Nov 18, 2015

hamedhsn commented Nov 18, 2015

jreback commented Nov 18, 2015

melroy89 commented Jan 18, 2017