ExtensionArray.shift incorrect for periods larger than length of array

[TomAugspurger]

In [9]: from pandas.tests.extension.decimal import to_decimal

In [10]: to_decimal([1, 2]).shift(4)
Out[10]:
DecimalArray(array([Decimal('NaN'), Decimal('NaN'), Decimal('NaN'), Decimal('NaN')],
      dtype=object))

The expected output is

>>> to_decimal(['NaN', 'NaN'])
DecimalArray(array([Decimal('NaN'), Decimal('NaN')], dtype=object))

similar to categorical

In [12]: pd.Categorical([1, 2]).shift(4)
Out[12]:
[NaN, NaN]
Categories (2, int64): [1, 2]

This will need a new base test in tests/extension/base/methods.py and a fix in

pandas/pandas/core/arrays/base.py

Line 430 in a7b187a

def shift(self, periods=1):

[Devilmoon]

Hi,

I would like to try and tackle this issue to bite my teeth into the pandas codebase, however I'm not entirely sure about the expected behaviour of the shift method.

From what I understand, when shift is called, the first period elements (up to the array's len) become NaN, and if period is smaller than the len of the array the original len-period elements become the last len-period elements, respecting the condition that period+len-period == len of the original array.

Is this correct?

[TomAugspurger]

@Devilmoon thanks. I think your summary is correct.

The implementation is correct when abs(period) < len(self). The only bug is when periods is larger (in absolute terms).

The issue is on

pandas/pandas/core/arrays/base.py

Line 454 in a7b187a

empty = self._from_sequence([self.dtype.na_value] * abs(periods),

That should be

        empty = self._from_sequence(
            [self.dtype.na_value] * min(abs(periods), len(self)),
            dtype=self.dtype
        )

[varadgunjal]

@TomAugspurger I'm trying to grok the correct way to put in a test for this.

I want to understand why a new base test is required under tests/extension/base/methods.py - wouldn't test_container_shift also cover testing the DecimalArray (generated by to_decimal) by casting it to a Series? Couldn't we just add the relevant test cases under @pytest.mark.parametrize and be done? Or am I thinking about this wrong?

[TomAugspurger]

We write base tests for things that should be true of all extension array implementations http://pandas-docs.github.io/pandas-docs-travis/extending.html#testing-extension-arrays

I think something like

def test_shift_large_periods(self, data, na_value):
    data = data[:5]
    result = data.shift(10)
    expected = type(data)._construct_from_sequence([na_value] * 10)
    self.assert_extension_array_equal(result, expected)

should work, but I haven't tested it. Maybe also test for periods=-10.

[varadgunjal]

Understood. Different question, why then should it be in that particular file which contains tests under the BaseMethodsTest class that are meant for Series and DataFrame methods?

[TomAugspurger]

It could also go under the interface tests.

…

On Mon, Nov 26, 2018 at 11:42 AM Varad Gunjal ***@***.***> wrote: Understood. Different question, why then should it be in that particular file which contains tests under the BaseMethodsTest class that are meant for Series and DataFrame methods? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23911 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHItGGYotleCMFBKt-a9pEH6lYpgSWks5uzCgcgaJpZM4YyCb_> .

[varadgunjal]

I followed a slightly different approach for the tests in the PR. Please let me know if that works.