Fixing shift() for ExtensionArray
State: closed

- [x] closes #23911 
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

Comments:
Hello @varadgunjal! Thanks for updating the PR.

 - There are no PEP8 issues in the file [`pandas/core/arrays/base.py`](https://github.com/varadgunjal/pandas/blob/172545a136ee2fbd7da46610f55eb4dbbfb49054/pandas/core/arrays/base.py) !

 - There are no PEP8 issues in the file [`pandas/core/arrays/sparse.py`](https://github.com/varadgunjal/pandas/blob/172545a136ee2fbd7da46610f55eb4dbbfb49054/pandas/core/arrays/sparse.py) !

 - There are no PEP8 issues in the file [`pandas/tests/extension/base/interface.py`](https://github.com/varadgunjal/pandas/blob/172545a136ee2fbd7da46610f55eb4dbbfb49054/pandas/tests/extension/base/interface.py) !



##### Comment last updated on November 27, 2018 at 20:04 Hours UTC
Looks like some of the other `shift` implementations  were buggy: https://travis-ci.org/pandas-dev/pandas/jobs/460168964

https://travis-ci.org/pandas-dev/pandas/jobs/46016896.

Could you also update the docstring for ExtensionArray.shift adding a note about the expected behavior when `periods` is larger than `self`?
So, this is the 2nd build I've seen fail. From what I understand from the logs, it looks like the previous implementation of `shift` is being used despite having pushed the update in the file.

Oops sorry, didn't read your comment before I posted the previous one.

> Could you also update the docstring for ExtensionArray.shift adding a note about the expected behavior when periods is larger than self?

Completely slipped my mind. Will do so.


> Looks like some of the other shift implementations were buggy.

So, what is the correct course of action? Should I dig in further and try to fix those or leave them for a separate PR?
ExtensionArray.shift is new in 0.24.0, so just need to make sure that
there's an existing note there.

On Tue, Nov 27, 2018 at 6:30 AM Jeff Reback <notifications@github.com>
wrote:

> *@jreback* requested changes on this pull request.
>
> can you add a whatsnew note, put in the extension array section
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/23947#pullrequestreview-178754742>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIg08SGNonHnbSrhdVs1nTtlA3HiYks5uzTBfgaJpZM4Y1Hv0>
> .
>

@TomAugspurger so I don't make an edit in whatsnew, correct? Also, how do I address the build failing on the newly added test?
@varadgunjal it seems like we don't have a dedicated release note for ExtensionArray.shift. Can you add one on ~938 and use `22387` for the issue?
Just noticed `22387` refers to a PR opened by you for issue #22386. Should I ignore that and keep it as the issue?
Either works.

On Tue, Nov 27, 2018 at 12:48 PM Varad Gunjal <notifications@github.com>
wrote:

> Just noticed 22387 refers to a PR opened by you for issue #22386
> <https://github.com/pandas-dev/pandas/issues/22386>. Should I ignore that
> and keep it as the issue?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/23947#issuecomment-442172424>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIrRIzObvX9TXsSkq_iyx5wVjzmyEks5uzYjngaJpZM4Y1Hv0>
> .
>

Cool. I still don't get what we're supposed to do about the failing build.though.
SparseArray defines `SparseArray.shift`. It seems it contains the same bug as the base `ExtensionArray.shift`, so you'll need to apply a similar fix there as well.
No, same PR. This is why we write base tests for these kinds of things, so
that every extension array implementation behaves correctly.
You don't need to write any new tests, since SparseArray automatically
picks up the one you wrote for this PR.

On Tue, Nov 27, 2018 at 1:33 PM Varad Gunjal <notifications@github.com>
wrote:

> 👍
>
> Separate PR I assume?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/23947#issuecomment-442187492>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHItLnjNQd29IfrrtUCy_J0F5g5S3Pks5uzZOAgaJpZM4Y1Hv0>
> .
>

The newest casualty is `ArrowBoolArray`. But I've run up against a problem when I tried to fix that, since the `assert values.type == pa.bool_()` constrains the values in it to be `boolean`. Thus, adding in an `na_value` violates this. So, what is the expected behavior here? Should I modify the `_from_sequence` implementation inside `ArrowBoolArray` to treat `NaN`s differently?
Feel free to add skips for the arrow tests. That implementation is pretty buggy right now.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23947?src=pr&el=h1) Report
> Merging [#23947](https://codecov.io/gh/pandas-dev/pandas/pull/23947?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/90961f2acad5c2db6aa957e2a79df8668f5b02d1?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/23947/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/23947?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #23947      +/-   ##
==========================================
+ Coverage   92.31%   92.31%   +<.01%     
==========================================
  Files         161      161              
  Lines       51483    51471      -12     
==========================================
- Hits        47525    47517       -8     
+ Misses       3958     3954       -4
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.71% <100%> (ø)` | :arrow_up: |
| #single | `42.43% <0%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/23947?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2Jhc2UucHk=) | `98.01% <100%> (+0.66%)` | :arrow_up: |
| [pandas/core/arrays/sparse.py](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3NwYXJzZS5weQ==) | `92.06% <100%> (+0.12%)` | :arrow_up: |
| [pandas/core/dtypes/cast.py](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2Nhc3QucHk=) | `89.38% <0%> (-0.25%)` | :arrow_down: |
| [pandas/io/json/normalize.py](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2pzb24vbm9ybWFsaXplLnB5) | `96.87% <0%> (ø)` | :arrow_up: |
| [pandas/tseries/offsets.py](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvb2Zmc2V0cy5weQ==) | `96.98% <0%> (ø)` | :arrow_up: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `96.47% <0%> (ø)` | :arrow_up: |
| [pandas/core/groupby/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9iYXNlLnB5) | `91.83% <0%> (ø)` | :arrow_up: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.02% <0%> (ø)` | :arrow_up: |
| [pandas/core/nanops.py](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvbmFub3BzLnB5) | `95.05% <0%> (ø)` | :arrow_up: |
| [pandas/io/json/json.py](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2pzb24vanNvbi5weQ==) | `93.09% <0%> (ø)` | :arrow_up: |
| ... and [14 more](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23947?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23947?src=pr&el=footer). Last update [90961f2...67c268d](https://codecov.io/gh/pandas-dev/pandas/pull/23947?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23947?src=pr&el=h1) Report
> Merging [#23947](https://codecov.io/gh/pandas-dev/pandas/pull/23947?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/90961f2acad5c2db6aa957e2a79df8668f5b02d1?src=pr&el=desc) will **decrease** coverage by `0.1%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/23947/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/23947?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #23947      +/-   ##
==========================================
- Coverage   92.31%    92.2%   -0.11%     
==========================================
  Files         161      162       +1     
  Lines       51483    51700     +217     
==========================================
+ Hits        47525    47671     +146     
- Misses       3958     4029      +71
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.6% <100%> (-0.1%)` | :arrow_down: |
| #single | `43.02% <0%> (+0.58%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/23947?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2Jhc2UucHk=) | `97.41% <100%> (+0.06%)` | :arrow_up: |
| [pandas/core/arrays/sparse.py](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3NwYXJzZS5weQ==) | `92.08% <100%> (+0.14%)` | :arrow_up: |
| [pandas/io/s3.py](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3MzLnB5) | `0% <0%> (-86.37%)` | :arrow_down: |
| [pandas/core/arrays/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3RpbWVkZWx0YXMucHk=) | `87.1% <0%> (-9.34%)` | :arrow_down: |
| [pandas/io/parquet.py](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3BhcnF1ZXQucHk=) | `76.92% <0%> (-7.7%)` | :arrow_down: |
| [pandas/io/formats/printing.py](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvcHJpbnRpbmcucHk=) | `85.41% <0%> (-7.67%)` | :arrow_down: |
| [pandas/io/formats/console.py](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvY29uc29sZS5weQ==) | `74.24% <0%> (-1.52%)` | :arrow_down: |
| [pandas/io/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2NvbW1vbi5weQ==) | `72.09% <0%> (-0.78%)` | :arrow_down: |
| [pandas/io/json/json.py](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2pzb24vanNvbi5weQ==) | `92.61% <0%> (-0.48%)` | :arrow_down: |
| ... and [69 more](https://codecov.io/gh/pandas-dev/pandas/pull/23947/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23947?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23947?src=pr&el=footer). Last update [90961f2...31a9cc8](https://codecov.io/gh/pandas-dev/pandas/pull/23947?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

@TomAugspurger thanks! Pushed the required changes.
@varadgunjal can you merge master and update to comments
> or are these just not running the methods tests?

This is the current behavior. The implementation of `ArrowBoolArray` does not run the methods tests at all.
Thanks @varadgunjal!