ENH: Added max_gap keyword for series.interpolate
State: closed

- [x] closes #12187 and #26796 
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

This PR introduces the new keyword `max_gap` for `interpolate`. For all NaN-gaps which are wider than `max_gap` no interpolation is carried out and all NaNs of the gap are preserved. This is in contrast to using the `limit` kwarg which does not prevent interpolating values in a longer NaN gap.

I added numpy-based implementation that searches for NaN-gaps wider than `max_gap`. In line with the current implementations for NaN handling in `series.interpolate`, a set of NaN-indices that has
to be preserved is generated. This is used in the end, after a full interpolation of all NaN is done, to restore the NaNs gaps that shall not be interpolated.

Test and documentation were also added.

It will need some small PEP8-cleanup and maybe tests using other interpolation methods then `linear` (edit: Done). But before I continue, I would like to get feedback if my approach is in general okay.

This PR might also be extended to close #16457 which is on interpolation directly after resampling.

Example usage:

```python
In [1]: import numpy as np                                                                                                                                                                                                         

In [2]: import pandas as pd                                                                                                                                                                                                        

In [3]: s = pd.Series([np.nan, 1., np.nan, 2., np.nan, np.nan,  
   ...:                5., np.nan, np.nan, np.nan, -1., np.nan, np.nan])                                                                                                                                                           

# Using the new `max_gap` kwarg
In [4]: s.interpolate(max_gap=2, limit_area='inside')                                                                                                                                                                               
Out[4]: 
0     NaN
1     1.0
2     1.5
3     2.0
4     3.0
5     4.0
6     5.0
7     NaN
8     NaN
9     NaN
10   -1.0
11    NaN
12    NaN
dtype: float64

# Compare to the result when using the existing `limit` kwarg
In [5]: s.interpolate(limit=2, limit_area='inside')                                                                                                                                                                                
Out[5]: 
0     NaN
1     1.0
2     1.5
3     2.0
4     3.0
5     4.0
6     5.0
7     3.5
8     2.0
9     NaN
10   -1.0
11    NaN
12    NaN
dtype: float64
```

Timing:
```python
In [6]: %timeit s.interpolate(max_gap=2, limit_area='inside')                                                                                                                                                                       
708 µs ± 14.5 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

In [7]: %timeit s.interpolate(limit=2, limit_area='inside')                                                                                                                                                                        
631 µs ± 5.85 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
```
 
The relative speed difference is similar for larger Series.

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

Comments:
Hello @cchwala! 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-11-20 14:00:08 UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25141?src=pr&el=h1) Report
> Merging [#25141](https://codecov.io/gh/pandas-dev/pandas/pull/25141?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/2e38d5552a5c7b2c0091cecddd483f4f08ad1d2c?src=pr&el=desc) will **decrease** coverage by `49.52%`.
> The diff coverage is `3.57%`.

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

```diff
@@             Coverage Diff             @@
##           master   #25141       +/-   ##
===========================================
- Coverage   92.37%   42.84%   -49.53%     
===========================================
  Files         166      166               
  Lines       52408    52430       +22     
===========================================
- Hits        48412    22464    -25948     
- Misses       3996    29966    +25970
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `?` | |
| #single | `42.84% <3.57%> (-0.02%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25141?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/25141/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `39.89% <ø> (-56.74%)` | :arrow_down: |
| [pandas/core/missing.py](https://codecov.io/gh/pandas-dev/pandas/pull/25141/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvbWlzc2luZy5weQ==) | `15.65% <3.57%> (-76.92%)` | :arrow_down: |
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/25141/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/core/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/25141/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/25141/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfY29uc3RhbnRzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/plotting.py](https://codecov.io/gh/pandas-dev/pandas/pull/25141/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvcGxvdHRpbmcucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/25141/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/25141/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/25141/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/25141/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXM3YmRhdC5weQ==) | `0% <0%> (-91.17%)` | :arrow_down: |
| ... and [125 more](https://codecov.io/gh/pandas-dev/pandas/pull/25141/diff?src=pr&el=tree-more) | |

------

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

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

```diff
@@            Coverage Diff            @@
##             master   #25141   +/-   ##
=========================================
  Coverage          ?   41.19%           
=========================================
  Files             ?      178           
  Lines             ?    50799           
  Branches          ?        0           
=========================================
  Hits              ?    20928           
  Misses            ?    29871           
  Partials          ?        0
```

| Flag | Coverage Δ | |
|---|---|---|
| #single | `41.19% <0%> (?)` | |


------

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

@jreback Just a quick comment to let you know that updating this PR is still on my TODO list, but I have not had time to work on it for the last two weeks. Next week looks better, at least judging from now...
can you merge master
Resolved the tiny merge conflict. Still no other progress with the PR. Sorry. I am too overloaded till early April.
@cchwala if you have time; this is a nice patch. pls update to comments.
@pandas-dev/pandas-core if someone wants to take this over the line, pls merge master and update to comments.
@jreback Sorry for the long silence. I will do it today.
There is a problem. For `s.interpolate(method='pad', max_gap=2)` the `max_gap` keyword does not seem to have an effect. I added a test which fails in this case.

From a first quick search the cause might lie here:
https://github.com/pandas-dev/pandas/blob/b8ad9da4213be55c587db015c640d927aeb7feaf/pandas/core/internals/blocks.py#L1096-L1116
There are two different pathways for `interpolate` depending on the selected `method`. For `pad`, `ffill` and `bfill` `missing.interpolate_2d` is used which does not yet support the `max_gap` option. It also does not seem to recognize the keywords `limit_direction` and `limit_area`, they are silently ignored.
@cchwala is this still active? Can you merge master
I resolved the merge conflict. However, I still have not found the time to work further on this PR, in particular because it somehow is blocked by #26796. It remains somewhere in the middle of my TODO list...
I have introduced a solution for #26796 so that the implementation of the new `max_gap` kwarg also works as expected when using `method=pad` . I put these changes all in this PR since they depend on the function `missing._derive_indices_of_nans_to_preserve`, which was introduced earlier in this PR.

The code now distinguishs between the 1D- and 2D-case of doing interpolation with `pad` or `backfill`. In case of 1D, the approach is similar to the one using the scipy-interpolate-machinery.

The code will need some polishing, but before I continue I want to know if the implementation is acceptable in general.

Questions to @jreback @TomAugspurger or others that are familiar with the interpolation code:
1. Should I continue with this approach?
1. Should I simplify the new [`missing.interpolate_1d_fill()`](https://github.com/pandas-dev/pandas/pull/25141/files/c72acdb904cc2875bd530a1b780a809082ab16bb..12d2e5b1408f058fa4eb09082e1472a58ecf4818#diff-0cca2dbee24098c1488ed79b05b7248fR521) to use [`pad_1d` and `backfill_1d`](https://github.com/pandas-dev/pandas/blob/12d2e5b1408f058fa4eb09082e1472a58ecf4818/pandas/core/missing.py#L679:L688) instead of the current approach using the [2D-code
](https://github.com/pandas-dev/pandas/blob/12d2e5b1408f058fa4eb09082e1472a58ecf4818/pandas/core/missing.py#L577:L582) that I copied from the original [`missing.interpolate_2d`](https://github.com/pandas-dev/pandas/blob/12d2e5b1408f058fa4eb09082e1472a58ecf4818/pandas/core/missing.py#L601)
I don't have a strong prior on whether this is a good approach, but your description sounds reasonable.
We're implementing this over at https://github.com/pydata/xarray/pull/3302. Do you have strong preferences on names? `limit_gap` or similar to align with the `limit` keyword? We'll follow you. 
No strong preference. Either of those sound reasonable.

On Thu, Sep 12, 2019 at 11:19 AM Maximilian Roos <notifications@github.com>
wrote:

> We're implementing this over at pydata/xarray#3302
> <https://github.com/pydata/xarray/pull/3302>. Do you have strong
> preferences on names? limit_gap or similar to align with the limit
> keyword? We'll follow you.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/25141?email_source=notifications&email_token=AAKAOIT4VCS7CSWQ3V3L7GDQJJTZ3A5CNFSM4GUFKSUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6SOJGI#issuecomment-530900121>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAKAOIRT56IHGRNBH2WOZGTQJJTZ3ANCNFSM4GUFKSUA>
> .
>

`maxgap` was proposed in the initial issue https://github.com/pandas-dev/pandas/issues/12187 and was taken from [`na.approx` in R](https://www.rdocumentation.org/packages/zoo/versions/1.8-6/topics/na.approx), which does the exact same thing. @jreback suggested to change to `max_gap`, which fits better with pandas kwarg naming convention.

In my opinion `max_gap` summarizes best what the kwarg does, it defines a maximum allowed gap width for interpolation. Every gap that is longer, stays untouched. With `limit_gap` this is not that clear, at least to me. Using the kwarg does not "limit" the gap.

Decisions are up to the maintainers, though.

By the way: Good to see this feature in xarray!
Yeah, let's do `max_gap`.

On Thu, Sep 12, 2019 at 1:31 PM Christian Chwala <notifications@github.com>
wrote:

> maxgap was proposed in the initial issue #12187
> <https://github.com/pandas-dev/pandas/issues/12187> and was taken from
> na.approx in R
> <https://www.rdocumentation.org/packages/zoo/versions/1.8-6/topics/na.approx>,
> which does the exact same thing. @jreback <https://github.com/jreback>
> suggested to change to max_gap, which fits better with pandas kwarg
> naming convention.
>
> In my opinion max_gap summarizes best what the kwarg does, it defines a
> maximum allowed gap width for interpolation. Every gap that is longer,
> stays untouched. With limit_gap this is not that clear, at least to me.
> Using the kwarg does not "limit" the gap.
>
> Decisions are up to the maintainers, though.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/25141?email_source=notifications&email_token=AAKAOIT6DB2AEG3TZID4T7LQJKDGXA5CNFSM4GUFKSUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6S2OHA#issuecomment-530949916>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAKAOISWO6FZEANT2QOR3ZTQJKDGXANCNFSM4GUFKSUA>
> .
>

Thanks a lot @cchwala & @TomAugspurger ! 
Digging further into this and adding more test for `method=pad` I ran into https://github.com/pandas-dev/pandas/issues/12918

Looking for a fix now...
Note to myself (and everybody interested):

There might be an inconsistency with how axes are determined in relation to the actual data stored for a DataFrame in `._data`, which might have reversed axes, indicated by `DataFrame._AXIS_REVERSED`.

[`NDFrame.interpolate()` has some logic to set the `axis`](https://github.com/pandas-dev/pandas/blob/367670e80ff008d77c791437a7a5df3e1abe012b/pandas/core/generic.py#L7001:L7009) while [`NDFrame.fillna()`](https://github.com/pandas-dev/pandas/blob/367670e80ff008d77c791437a7a5df3e1abe012b/pandas/core/generic.py#L6232:L6233) just sets a default. Both call `NDFrame._data.interpolate()` then using the axis. 

When using `DataFrame.pad()` vs `DataFrame.interpolate(method=`pad`) the `axis` in `NDFrame._data.interpolate()` are different. Unfortunately some simple fixes that I tried made several other test fail. So no fix for now...


FYI, I am offline till 7th of October but plan to continue with this PR afterwards
@cchwala is this still active?
@WillAyd Not really active... But I resolved the merge conflicts on my machine.

Unfortunately I am struggling with ~~#29330~~ a conda problem on my machine, even in a fresh miniconda3 install, so I cannot run the tests. I will push, when I have verified that my merge makes sense.
@TomAugspurger @WillAyd This is now finally ready for another review.
> Is there any chance that the changes to allow `limit_area` and `limit_direction` in Series.interpolate with `pad` can be split into its own PR? Would that make this one much smaller?

For making `max_gap` work with `pad` I had to introduce [missing.interpolate_1d_fill](https://github.com/pandas-dev/pandas/pull/25141/files#diff-0cca2dbee24098c1488ed79b05b7248fR537) because the existing`missing.interpolate_2d`, which `pad` did always use, is not (easily) extendable. The cause for the wrong behavior of `limit_area` when using `pad` was the same, so this was solved somehow as a byproduct.

If we would want to split this PR up, probably most changes of this PR would move to the one to solve the issue with `pad` and `limit_area`. So yes, this one would be much smaller, but the other one would be equally large.


@cchwala can you merge master and fix up the CI error for code checks?
@WillAyd I separated large parts of the changes into #31048 and will continue here afterwards.
Closing to clean queue - I think can revisit after #31048