ENH: fill_value argument for shift #15486
State: closed

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


Labels:
name: Enhancement
name: Missing-data
description: np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode

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































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

##### Comment last updated on December 26, 2018 at 00:17 Hours UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24128?src=pr&el=h1) Report
> Merging [#24128](https://codecov.io/gh/pandas-dev/pandas/pull/24128?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/f6cf7d9e2606e82fd3934e1dac8119c43f5ee643?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #24128      +/-   ##
==========================================
+ Coverage   92.29%    92.3%   +0.01%     
==========================================
  Files         162      163       +1     
  Lines       51832    51978     +146     
==========================================
+ Hits        47839    47980     +141     
- Misses       3993     3998       +5
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.71% <100%> (+0.01%)` | :arrow_up: |
| #single | `42.99% <23.33%> (+0.01%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24128?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `93.72% <ø> (+0.01%)` | :arrow_up: |
| [pandas/core/arrays/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2Jhc2UucHk=) | `97.59% <100%> (+0.17%)` | :arrow_up: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.91% <100%> (ø)` | :arrow_up: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `96.62% <100%> (-0.04%)` | :arrow_down: |
| [pandas/core/arrays/period.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3BlcmlvZC5weQ==) | `98.48% <100%> (-0.02%)` | :arrow_down: |
| [pandas/core/internals/blocks.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL2Jsb2Nrcy5weQ==) | `93.81% <100%> (ø)` | :arrow_up: |
| [pandas/core/groupby/groupby.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9ncm91cGJ5LnB5) | `96.8% <100%> (+0.15%)` | :arrow_up: |
| [pandas/core/arrays/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2NhdGVnb3JpY2FsLnB5) | `95.34% <100%> (+0.02%)` | :arrow_up: |
| [pandas/core/arrays/sparse.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3NwYXJzZS5weQ==) | `92.1% <100%> (+0.01%)` | :arrow_up: |
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `97.6% <0%> (-0.64%)` | :arrow_down: |
| ... and [25 more](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24128?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/24128?src=pr&el=footer). Last update [f6cf7d9...a03cbf5](https://codecov.io/gh/pandas-dev/pandas/pull/24128?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/24128?src=pr&el=h1) Report
> Merging [#24128](https://codecov.io/gh/pandas-dev/pandas/pull/24128?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/b8413746d9d4c4ba6f7a1e694eabf7351f22b1a1?src=pr&el=desc) will **decrease** coverage by `49.17%`.
> The diff coverage is `58.33%`.

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

```diff
@@             Coverage Diff             @@
##           master   #24128       +/-   ##
===========================================
- Coverage    92.2%   43.03%   -49.18%     
===========================================
  Files         162      162               
  Lines       51701    51701               
===========================================
- Hits        47672    22247    -25425     
- Misses       4029    29454    +25425
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `?` | |
| #single | `43.03% <58.33%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24128?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `35.88% <100%> (-61.03%)` | :arrow_down: |
| [pandas/core/internals/blocks.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL2Jsb2Nrcy5weQ==) | `52.93% <50%> (-40.79%)` | :arrow_down: |
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `49.32% <50%> (-44.38%)` | :arrow_down: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `39.65% <50%> (-57%)` | :arrow_down: |
| [pandas/core/arrays/datetimelike.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lbGlrZS5weQ==) | `48.74% <66.66%> (-47.61%)` | :arrow_down: |
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/core/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY2F0ZWdvcmljYWwucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/sas/sas\_constants.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfY29uc3RhbnRzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/plotting.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvcGxvdHRpbmcucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvY29udmVydGVyLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| ... and [124 more](https://codecov.io/gh/pandas-dev/pandas/pull/24128/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24128?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/24128?src=pr&el=footer). Last update [b841374...8b5cedb](https://codecov.io/gh/pandas-dev/pandas/pull/24128?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

added fill_value to shift implementations across different classes, according to the failures from tests
@jreback I made all the changes, except the Index subclasses, because I think Index doesn't need fill_value. it does the shift correctly already with the freq specified
can you review?
it seems that we prohibit usage of `np.testing` in the pandas tests. @TomAugspurger, @jreback do you know what I can use instead?
UPDATE: I replaced numpy tests with pandas comparison of the series and frames
At the moment, Azure seems to fail randomly and with errors that I don't really understand...
I would appreciate any help or comment to make it pass
I think there are a few things not tested

- Categorical.shift with non default fill_value
- Categorical.shift where fill_value raises on not present
- SparseArray.shift with non default fill value
- Series.shift with freq and non-default fill value
- Groupby.shift with non-default fill value
Series.shift is tested I think with `pandas/tests/series/test_timeseries.py:138`
I will work on adding the rest
added the tests
@jreback @TomAugspurger please review when you have time, I added all the missing tests. It would be great if we can merge this soon
There is some error in Travis that is probably not related to my changes...
did all the requested changes
all tests passed  @jreback can you review, please?
please let me know
@ahcub will have a look when a chance
test for dtype was added and parametrization for the test in test_array.py was changed @TomAugspurger 
Looks pretty close. Just a small comment on the sparse stuff.
A base test for extension arrays would be nice. Can go in
`extension/base/methods.py`.

On Tue, Dec 18, 2018 at 4:01 PM Jeff Reback <notifications@github.com>
wrote:

> *@jreback* requested changes on this pull request.
>
> are there tests for extension arrays? (aside from sparse)
> ------------------------------
>
> In pandas/core/arrays/categorical.py
> <https://github.com/pandas-dev/pandas/pull/24128#discussion_r242719966>:
>
> > @@ -1275,10 +1279,18 @@ def shift(self, periods):
>              raise NotImplementedError("Categorical with ndim > 1.")
>          if np.prod(codes.shape) and (periods != 0):
>              codes = np.roll(codes, ensure_platform_int(periods), axis=0)
> +            if isna(fill_value):
> +                fill_value = -1
> +            elif fill_value in self.categories:
> +                fill_value = self.categories.get_loc(fill_value)
>
> is there a test for this (should raise KeyError)
> ------------------------------
>
> In pandas/core/arrays/categorical.py
> <https://github.com/pandas-dev/pandas/pull/24128#discussion_r242720003>:
>
> > @@ -1275,10 +1279,18 @@ def shift(self, periods):
>              raise NotImplementedError("Categorical with ndim > 1.")
>          if np.prod(codes.shape) and (periods != 0):
>              codes = np.roll(codes, ensure_platform_int(periods), axis=0)
> +            if isna(fill_value):
> +                fill_value = -1
>
> test for this?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/24128#pullrequestreview-186300345>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIqL5Ih5QBp5k5nF9mQZ-UPXTEQKcks5u6WW5gaJpZM4ZGVID>
> .
>

we need to retrigger azure tests suite because the error there doesn't seem to be related to the changes
Please let me know if anything else needs to be changed for this. It's been awhile since the PR started and it would be cool to merge it sometime soon
Looks like theres a merge conflict.

Can you add a base test in `pandas/tests/extension/base/methods.py`?

Let me know if you need help writing one.
Merge conflict is resolved.
Regarding the base test, sorry, I'm not sure which functionality you mean, and if possible I would prefer to avoid adding it in this PR if it is not a problem.
We’ll need it to ensure that extension arrays handle the argument correctly. I’ll take a look later.


________________________________
From: Alexander Buchkovsky <notifications@github.com>
Sent: Wednesday, December 19, 2018 19:00
To: pandas-dev/pandas
Cc: Tom Augspurger; Mention
Subject: Re: [pandas-dev/pandas] ENH: fill_value argument for shift #15486 (#24128)


Merge conflict is resolved.
Regarding the base test, sorry, I'm not sure which functionality you mean, and if possible I would prefer to avoid adding it in this PR if it is not a problem.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://github.com/pandas-dev/pandas/pull/24128#issuecomment-448815137>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABQHItuYCNTXZxbrASF3dFP7AjeouQAGks5u6uFJgaJpZM4ZGVID>.

Pushed 6c9610839 with a base test.
cool, thanks
@jreback can you please check the PR one more time?
Please let me know. This feature would make my life a lot simpler, because I'm using the shift with fillna often and it changes the dtype of the series, which I have to adjust every time.

One last linting error as well:

```
2018-12-25T18:31:36.1417763Z ERROR: /home/vsts/work/1/s/pandas/tests/arrays/sparse/test_array.py Imports are incorrectly sorted.
2018-12-25T18:31:36.1417924Z ERROR: /home/vsts/work/1/s/pandas/core/arrays/base.py Imports are incorrectly sorted.
```
used isort to fix those, hopefully no issues this time 🙏