Fix/time series interpolation is wrong 21351
State: closed

- [x] closes #21351
- [x] [Tests added and passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#writing-tests) if fixing a bug or adding a new feature
- [x] All [code checks passed](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#pre-commit).
- [x] Added [type annotations](https://pandas.pydata.org/pandas-docs/dev/development/contributing_codebase.html#type-hints) to new arguments/methods/functions.
- [x] Added an entry in the latest `doc/source/whatsnew/vX.X.X.rst` file if fixing a bug or adding a new feature.


Labels:
name: Missing-data
description: np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate
name: Error Reporting
description: Incorrect or improved errors from pandas

Comments:
> FAILED pandas/tests/resample/test_time_grouper.py::test_groupby_resample_interpolate - TypeError: '<' not supported between instances of 'int' and 'tuple'

Any idea what is causing this?  Seems unlikely to be "`expected` is wrong" for this one (which i assume is the case for the other one)
IIUC in the motivating case we have a series whose index has freq1 and a new freq2 that are not multiples of each other.  Is what you're doing here similar to

```
freq3 = LCD(freq1, freq2)
result = series.resample(freq3).interpolate(...).asfreq(freq2)
```

And if so, would that be more efficient/clearer than what you're doing here?  I guess it wouldn't work if the original index doesn't have a freq?  Or might blow up if the LCD happens to be tiny?
is there a doctest this changes/fixes?
> IIUC in the motivating case we have a series whose index has freq1 and a new freq2 that are not multiples of each other. Is what you're doing here similar to
> 
> ```
> freq3 = LCD(freq1, freq2)
> result = series.resample(freq3).interpolate(...).asfreq(freq2)
> ```
> 
> And if so, would that be more efficient/clearer than what you're doing here? I guess it wouldn't work if the original index doesn't have a freq? Or might blow up if the LCD happens to be tiny?

@jbrockmendel  I don't think this is a "pathological" case, but in general, this type of resampling should be agnostic to what sort of index it is. 

It is helpful to think of the input data as a "measurement" of a function $f(x)$ at some points $x_i$, and the resampling/interpolation as asking the question about function values at a "higher number" of points $x_j$. The code identifies this correctly as **up**sampling. The interpolator is a "piecewise assumed function" $g(x)$ that (hopefully) describes how the data behaves between adjacent points $x_i, x_{i+1}$. It is irrelevant whether the original sampling followed a precise frequency. Instead, it is important that the interpolator can take into account as many points in the vicinity of the new points $x_j$ to evaluate $g$. (In the simple linear case 2 adjacent points are enough, but for more complex interpolation more anchor points may be used.)

The problem with the previous implementation is that those anchor points were, more or less randomly, excluded from the interpolation process. All we need to do is make sure none of the original data points is missing because this would mean throwing away valid information. This is achieved by
~~~python
result = concat(
    [result, obj.loc[missing_data_points_index]]
)
~~~
The remaining code is just gymnastics around this. I can help make this more clear in the code or here, but I don't think it can be simplified further.
> > FAILED pandas/tests/resample/test_time_grouper.py::test_groupby_resample_interpolate - TypeError: '<' not supported between instances of 'int' and 'tuple'
> 
> Any idea what is causing this? Seems unlikely to be "`expected` is wrong" for this one (which i assume is the case for the other one)

@jbrockmendel I spent quite a while looking at this now. The problem here is caused by interpolation applied to a `GroupBy` object. I was not able to solve how I would access/recreate the `MultiIndex` that the original data frame would have in `Resampler.interpolate` if it was not resampled 😞. The resampling happens somewhere in `self._upsample`. So there is a lack of information here, in a sense. I think I would need help to make it work for the groupby case, and it would feel unnatural to support it for non-grouped, but not grouped operations.
@jbrockmendel I took another look and added an implementation for multi-indexes (resulting from groupby). This fixes `test_groupby_resample_interpolate`, all tests in `resample` are green for me now locally.
And regarding `pandas.tests.series.methods.test_interpolate.TestSeriesInterpolateData.test_interpolate`, which seems to be the test that crashes in most of the other test suites: I don't see how this test was ever passing 🤷‍♂️. The input `datetime_series` has an index created using `date_range` with `freq="B"`. So this index is not even spaced, as it has only business days. A linear interpolation will therefore never result in `np.arange(len(datetime_series), dtype=float)`.
thanks for your PR

there's still some failing tests:
```
FAILED pandas/tests/series/methods/test_interpolate.py::TestSeriesInterpolateData::test_interpolate - AssertionError: Series are different

Series values are different (13.33333 %)
[index]: [2000-01-03T00:00:00.000000000, 2000-01-04T00:00:00.000000000, 2000-01-05T00:00:00.000000000, 2000-01-06T00:00:00.000000000, 2000-01-07T00:00:00.000000000, 2000-01-10T00:00:00.000000000, 2000-01-11T00:00:00.000000000, 2000-01-12T00:00:00.000000000, 2000-01-13T00:00:00.000000000, 2000-01-14T00:00:00.000000000, 2000-01-17T00:00:00.000000000, 2000-01-18T00:00:00.000000000, 2000-01-19T00:00:00.000000000, 2000-01-20T00:00:00.000000000, 2000-01-21T00:00:00.000000000, 2000-01-24T00:00:00.000000000, 2000-01-25T00:00:00.000000000, 2000-01-26T00:00:00.000000000, 2000-01-27T00:00:00.000000000, 2000-01-28T00:00:00.000000000, 2000-01-31T00:00:00.000000000, 2000-02-01T00:00:00.000000000, 2000-02-02T00:00:00.000000000, 2000-02-03T00:00:00.000000000, 2000-02-04T00:00:00.000000000, 2000-02-07T00:00:00.000000000, 2000-02-08T00:00:00.000000000, 2000-02-09T00:00:00.000000000, 2000-02-10T00:00:00.000000000, 2000-02-11T00:00:00.000000000]
[left]:  [0.0, 1.0, 2.0, 3.0, 4.0, 5.8, 6.4, 7.0, 7.6, 8.2, 10.0, 11.0, 12.0, 13.0, 14.0, 15.0, 16.0, 17.0, 18.0, 19.0, 20.0, 21.0, 22.0, 23.0, 24.0, 25.0, 26.0, 27.0, 28.0, 29.0]
[right]: [0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0, 10.0, 11.0, 12.0, 13.0, 14.0, 15.0, 16.0, 17.0, 18.0, 19.0, 20.0, 21.0, 22.0, 23.0, 24.0, 25.0, 26.0, 27.0, 28.0, 29.0]
At positional index 5, first diff: 5.8 != 5.0
FAILED pandas/tests/series/methods/test_interpolate.py::TestSeriesInterpolateData::test_nan_irregular_index - AssertionError: Series are different
```

If these were wrong to begin with, could you please updated them so they have the correct values so that they pass? That would need doing as part of this PR

CI would need to be green for this to be mergeable (furthermore, people are much more likely to review a PR which is green)
All green now @MarcoGorelli @jbrockmendel 
thanks - aiming to get to this this month
@MarcoGorelli any updates on this? :) 
will try to get this in by 3.0, got quite a few others things going on i'm afraid sorry
@MarcoGorelli nice catch! The problem is fixed now and tested in an existing test case. Synced with `upstream/main` and made an additional change that caused errors because of changes in the `scipy.interpolate` wrapper. Should be all good now.
@MarcoGorelli something just went wrong with the Numpy Dev test, but it is unrelated to the test cases. You may check that out because it could be relevant for `main` as well. I also don't seem to be able to re-run the flow, would need to push a dummy commit.
@MarcoGorelli Really have no idea what's going on with these test cases:
![image](https://github.com/pandas-dev/pandas/assets/19518682/38a9337a-03b3-4558-9bf3-fd048a7eee09)

Help would be appreciated 😕 
> Thanks for updating
> 
> The single-index part looks good
> 
> The multi-index part kind of scares me, I'm pretty sure it's going to break

> For example:
> 
> ```python
> In [10]: df = pd.DataFrame({'price': [float('nan'), 2., 3., float('nan'), 4., 5.], 'group': [1,1,1,2,2,2]}, index=pd.date_range('2018-01-07', periods=6))
> 
> In [11]: df
> Out[11]: 
>             price  group
> 2018-01-07    NaN      1
> 2018-01-08    2.0      1
> 2018-01-09    3.0      1
> 2018-01-10    NaN      2
> 2018-01-11    4.0      2
> 2018-01-12    5.0      2
> ```
> 
> If I did `df.groupby('group').resample('1d').interpolate()` I'd expect it to resample to daily and interpolate only within each group. But instead, it ignores the group boundaries?
> 
> ```
>                   price  group
> group                         
> 1     2018-01-07    NaN      1
>       2018-01-08    2.0      1
>       2018-01-09    3.0      1
> 2     2018-01-10    3.5      2
>       2018-01-11    4.0      2
>       2018-01-12    5.0      2
> ```


This is weird indeed. I debugged this, as it is unexpected. It somehow constructs an invalid index here: https://github.com/pandas-dev/pandas/blob/main/pandas/core/generic.py#L7851
It ignores the Multiindex and just assumes the index is `Index([0, 1, 2, 3, 4, 5], dtype='int64')`.

This is code I have not touched and I confirmed that **it is actually the old behavior**! You can try on pandas 2 stable and will see the same result being printed. This means, my fix somehow does not apply to this case, but it is also not introducing anything that fails or Is more wrong than it was before.

> 
> But my question is - do we really need it? It doesn't look to me like it does the right thing in the first place anyway
> 
> How about we just raise `NotImplementedError` in the `if isinstance(final_index, MultiIndex):` branch?

Your question is related to the target feature set and behavior of pandas, which is something I am not entitled to answer. Should I raise the error or fix the implementation?
> This is code I have not touched and I confirmed that it is actually the old behavior! You can try on pandas 2 stable and will see the same result being printed. This means, my fix somehow does not apply to this case, but it is also not introducing anything that fails or Is more wrong than it was before.

> Your question is related to the target feature set and behavior of pandas, which is something I am not entitled to answer. Should I raise the error or fix the implementation?

True, but if I understand correctly we're adding a fair deal of complexity in this PR to deal with that already-broken case

I'll bring this up in the dev call on Wedesnday (which btw is open to anyone, if you're interested), but I think it'll be fine to just raise - either here or in a precursor PR
@MarcoGorelli I tried to handle the `groupby(...).resample(...).interpolate(...)` case again, but I am now relatively sure that it cannot be handled given the current framework. This means I agree, we should raise an error in the case of a `Multiindex` seen in `pandas.core.resample.Resampler.interpolate`. We should recommend to interpolate each group separately, as the operations are independent anyway.

Disclaimer: with "time" being probably the most important independent variable and group-by an important operation, this will probably lead to some user frustration. For a user it will be implausible why this is not working.

Internally, the main problem why it cannot be done without larger modifications is the way how the input data arrives in `pandas.core.resample.Resampler.interpolate`. The original input data structure is effectively lost. Even when accessing the private attributes of `self` (e.g. `_groups` or `_selected_obj`), it seems to be impossible at this stage to tell which data points had been there in the beginning of the processing chain. Esp. so if there were duplicate timestamps, which can happen easily if they are unique per group. Because of that, we cannot know if the result of `self._upsample("asfreq")` has missing rows in the case of `Multiindex`, inputs. But this is essential.

If you give me a final go to raise the error and support only flat Datetimeindex, I will make the changes. Unfortunately, I am travelling on Wednesday so cannot join the call. But would love to join a following one.


thanks for looking into this!

cool, sounds good. I'll let you know if anything unexpected comes up at the call, but imagine that people will be cool with this approach
@MarcoGorelli Nice :) One more thing: the correct behavior can actually be obtained by just using `apply`. So we could output a very helpful error message, pointing the user at an alternative way to achieve the correct behavior. Taking your example and making sure there is a missing index:

~~~python
>>> df = DataFrame(
    {
        "price": [float("nan"), 2.0, 3.0, float("nan"), 4.0, float("nan"), 6.0],
        "group": [1, 1, 1, 2, 2, 2, 2],
    },
    index=date_range("2018-01-07", periods=7),
)
>>> df.drop(df.index[5], inplace=True)
>>> df
~~~
prints:
~~~python
            price  group
2018-01-07    NaN      1
2018-01-08    2.0      1
2018-01-09    3.0      1
2018-01-10    NaN      2
2018-01-11    4.0      2
2018-01-13    6.0      2
~~~
Note that `2018-01-12` is missing. Then using
~~~python
>>> df.groupby("group").apply(lambda x: x.resample("1d").interpolate(), include_groups=False)
~~~
gives the correct result:
~~~python
                  price
group                  
1     2018-01-07    NaN
      2018-01-08    2.0
      2018-01-09    3.0
2     2018-01-10    NaN
      2018-01-11    4.0
      2018-01-12    5.0
      2018-01-13    6.0
~~~

cool, definitely in favour of helpful error messages pointing to alternatives 🙌 
@MarcoGorelli please have a look again, it is now what we agreed on 😃 
I don't think the failing test cases are related to my code @MarcoGorelli @mroeschke , what do I do in this case? It occurred after syncing with `main`.
Thanks @cbpygit