REGR: Fix interpolation on empty dataframe

[sanderland]

Interpolation on an empty dataframe broke in 1.1 due to a change in how 'all columns are objects' is checked (specifically all(empty set) is True, while before dtype count object = None was checked against size = 0).
This is a complex function and I'm not sure what the proper fix is, suggesting to keep the empty check out of the rest of the logic.

Example code that broke:

import pandas as pd
df = pd.DataFrame([1,2])
df[[]].interpolate(limit_area='inside')

• closes BUG: interpolate gives a TypeError on empty dataframes #35598
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

[pep8speaks]

Hello @sanderland! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

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

Comment last updated at 2020-08-07 19:37:15 UTC

[WillAyd]

Can you add a test case for this?

[sanderland]

Can you add a test case for this?

done

[jreback]

i guess this was a regression, can you add a note in 1.1.1

[jreback]

would prefer that this happens in the internal method itself (called on L6861)

[sanderland]

The error happens before this call (on line 6825)

[simonjayhawkins]

There were a few changes to interpolate in 1.1.0. will run git bisect to ascertain why this is now failing.

[simonjayhawkins]

There was a subtle change in #34752 where

if self.ndim == 2 and np.all(self.dtypes == np.dtype(object)):

was changed to

if obj.ndim == 2 and np.all(obj.dtypes == np.dtype(object)):

does reverting this change restore the old behaviour.

[simonjayhawkins]

the regression is from #33084, but since the error message is about DataFrame columns, I don't think the above should have been changed either. @jbrockmendel

[jorisvandenbossche]

would prefer that this happens in the internal method itself (called on L6861)

As @sanderland notes, the check for all object dtype indeed happens here before calling into the internal method, so the check for empty thus also needs to happen here

However, @sanderland I think the df.empty check is not fully correct. Because this was also give True if you have columns but no rows. And for example an empty dataframe with a datetime64[ns] column will give a timedelta64[ns] column as result. That's something we should keep.

So I think we should explicitly check for no columns / no rows (alternatively could also add the check to the offending if obj.ndim == 2 and np.all(obj.dtypes == np.dtype(object)):, eg .. and obj.shape[0])

[jreback]

@sanderland can you try @jorisvandenbossche suggestion here

[sanderland]

However, @sanderland I think the df.empty check is not fully correct. Because this was also give True if you have columns but no rows. And for example an empty dataframe with a datetime64[ns] column will give a timedelta64[ns] column as result. That's something we should keep.

I tried this in 1.0.x and it does not return a timedelta dtype, I dont see why interpolate would do this either. Could you suggest a test which fails with my approach but passes in 1.0.x?

[jorisvandenbossche]

Hmm, you're fully correct. I might have been mixing up the review of two PRs, as I was also reviewing a regression for diff() and maybe was testing that method here as well .. ;) (since df.diff() for datetime64 gives timedelta64, also for empty dataframe)

Forget my comment!

[jorisvandenbossche]

@sanderland can you merge master once more to see if that fixes CI?

[simonjayhawkins]

test failure was

=========================== short test summary info ===========================
FAILED pandas/tests/computation/test_eval.py::TestAlignment::test_basic_series_frame_alignment[numexpr-python]
= 1 failed, 72824 passed, 1746 skipped, 1055 xfailed, 58 warnings in 1102.57s (0:18:22) =

probably unrelated.

restarted (azure will patch against master)

[simonjayhawkins]

Thanks @sanderland

[simonjayhawkins]

@meeseeksdev backport 1.1.x

[jreback]

@simonjayhawkins in the future pls don't merge these so fast

this is not what we wnat here

eg this should actually return self.copy()

not to mention this is not the correct place for this

[jorisvandenbossche]

I think Simon's call to merge this was appropriate (all the existing review comments were addressed, it was reviewed by several core devs). Yes, you noticed an additional problem (the missing copy) after merge: no problem, that can always happen, and we can do a follow-up to fix this.

not to mention this is not the correct place for this

See my comment at #35543 (comment) (and @sanderland's own answer above that), your suggestion to move it down into the internals is not possible, so this function is a correct place to put the check.