Series pct_change fill_method behavior
State: closed

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


Labels:
name: Missing-data
description: np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate
name: API Design

Comments:
Hello @albertvillanova! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:













There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2019-08-30 16:38:20 UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25291?src=pr&el=h1) Report
> Merging [#25291](https://codecov.io/gh/pandas-dev/pandas/pull/25291?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/4d44a2aff08c3075d9472e361bd7448a0472494e?src=pr&el=desc) will **decrease** coverage by `50%`.
> The diff coverage is `16.66%`.

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

```diff
@@             Coverage Diff             @@
##           master   #25291       +/-   ##
===========================================
- Coverage   91.72%   41.71%   -50.01%     
===========================================
  Files         173      173               
  Lines       52831    52841       +10     
===========================================
- Hits        48457    22045    -26412     
- Misses       4374    30796    +26422
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `?` | |
| #single | `41.71% <16.66%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25291?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/25291/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `38.24% <16.66%> (-55.93%)` | :arrow_down: |
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/25291/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/core/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/25291/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/25291/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfY29uc3RhbnRzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/plotting.py](https://codecov.io/gh/pandas-dev/pandas/pull/25291/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvcGxvdHRpbmcucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/25291/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvY29udmVydGVyLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/formats/html.py](https://codecov.io/gh/pandas-dev/pandas/pull/25291/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvaHRtbC5weQ==) | `0% <0%> (-99.35%)` | :arrow_down: |
| [pandas/core/groupby/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/25291/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9jYXRlZ29yaWNhbC5weQ==) | `0% <0%> (-95.46%)` | :arrow_down: |
| [pandas/io/sas/sas7bdat.py](https://codecov.io/gh/pandas-dev/pandas/pull/25291/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXM3YmRhdC5weQ==) | `0% <0%> (-91.17%)` | :arrow_down: |
| [pandas/io/sas/sas\_xport.py](https://codecov.io/gh/pandas-dev/pandas/pull/25291/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfeHBvcnQucHk=) | `0% <0%> (-90.15%)` | :arrow_down: |
| ... and [130 more](https://codecov.io/gh/pandas-dev/pandas/pull/25291/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25291?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/25291?src=pr&el=footer). Last update [4d44a2a...4418bf1](https://codecov.io/gh/pandas-dev/pandas/pull/25291?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/25291?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@7b25463`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff            @@
##             master   #25291   +/-   ##
=========================================
  Coverage          ?   91.68%           
=========================================
  Files             ?      174           
  Lines             ?    50751           
  Branches          ?        0           
=========================================
  Hits              ?    46531           
  Misses            ?     4220           
  Partials          ?        0
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.19% <100%> (?)` | |
| #single | `41.15% <7.14%> (?)` | |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25291?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/25291/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `93.38% <100%> (ø)` | |

------

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

@jreback all checks have passed.
@WillAyd @jschendel could you please have a look?
I guess I had a pending review that wasn't submitted:

Is anyone else uncomfortable with making `skipna=True` the default? If I have a timeseries, like

```python
In [9]: s = pd.Series([90, 91, None, 85, None, 95], index=pd.date_range('2000', periods=6))

In [10]: s
Out[10]:
2000-01-01    90.0
2000-01-02    91.0
2000-01-03     NaN
2000-01-04    85.0
2000-01-05     NaN
2000-01-06    95.0
Freq: D, dtype: float64

In [11]: s.pct_change()
Out[11]:
2000-01-01         NaN
2000-01-02    0.011111
2000-01-03         NaN
2000-01-04   -0.065934
2000-01-05         NaN
2000-01-06    0.117647
Freq: D, dtype: float64
```

Ideally, we would read the output as "the percent change between two periods". But, e.g. for the output labels 2000-01-04 the value is -.0.659; That's the percent change between 2000-01-02 and 2000-01-04. I know that's what we do on master, so maybe it's not a big deal... 

To me, the most useful thing with `.pct_chnage` and missing data is for the output to be missing when either the previous or current period is NaN. Can we achieve that with `skipna=False`? Is that documented?
@TomAugspurger yes, the behavior you are interested in can be achieved by setting `skipna=False`:

```
In [11]: s = pd.Series([90, 91, None, 85, None, 95, 97], index=pd.date_range('2000', periods=7))                                               

In [12]: s                                                                                                                                     
Out[12]: 
2000-01-01    90.0
2000-01-02    91.0
2000-01-03     NaN
2000-01-04    85.0
2000-01-05     NaN
2000-01-06    95.0
2000-01-07    97.0
Freq: D, dtype: float64

In [13]: s.pct_change(skipna=False)                                                                                                            
Out[13]: 
2000-01-01         NaN
2000-01-02    0.011111
2000-01-03         NaN
2000-01-04         NaN
2000-01-05         NaN
2000-01-06         NaN
2000-01-07    0.021053
Freq: D, dtype: float64
```

I totally agree with you that the behavior with `skipna=False` is much more sensible and intuitive.
@albertvillanova can you merge master and address latest comments?
can you merge master
I think only a small comment left, can you merge master and respond.
can you merge master here
Rebased to keep current. Let's see if we can get this one in
Seems that CI is failing (haven't looked closely).

Is the release note accurate (just an enhancement, not an API change?)

Looks like we need a docstring example with `skipna=False`.


Updated docstring, though on closer look this does change the default behavior against master:

before change:

```python
>>> s = pd.Series([90, 91, np.nan, 85, np.nan, 95])
>>> s.pct_change()
0         NaN
1    0.011111
2    0.000000
3   -0.065934
4    0.000000
5    0.117647
dtype: float64
```

this branch:
```python
>>> s = pd.Series([90, 91, np.nan, 85, np.nan, 95])
>>> s.pct_change()
0         NaN
1    0.011111
2         NaN
3   -0.065934
4         NaN
5    0.117647
dtype: float64
```

So I think need to be careful here
Hmm thanks for checking. This seems like something we can do in a backwards-compatible way (possibly with a deprecation).
I've personally put this on the back burner for now - @albertvillanova are you interested in picking back up?
I think this requires some effort to manage the backwards compat piece but not something I personally have time / motivation to dedicate to. Looks stale otherwise so closing, but @albertvillanova if something you'd like to pick back up please ping and we can continue on!