fix bfill, ffill and pad when calling with df.interpolate with column… #33959

CloseChoice · 2020-05-03T23:38:37Z

… dtype string (#33956)

closes BUG: throwing error in interpolate depending on dtype of column names #33956
closes DataFrame.ffill behaves different than DataFrame.interpolate(method='ffill') along axes #12918
closes df.interpolate(method='pad') axis is not consistent with df.fillna(method='pad') #29146
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

simonjayhawkins

Thanks @CloseChoice for the PR.

not sure we should be adding tests for incorrect/undoumented behaviour. see #33956 (comment)

simonjayhawkins · 2020-05-12T12:51:51Z

xref #12918 maybe the tests could test for equality with either DataFrame.ffill or DataFrame.fillna(method='ffill) and update the DataFrame.interpolate docstring, see #12918 (comment) and #12918 (comment)

jreback · 2020-05-12T16:13:41Z

can you merge master

CloseChoice · 2020-05-13T20:07:00Z

xref #12918 maybe the tests could test for equality with either DataFrame.ffill or DataFrame.fillna(method='ffill) and update the DataFrame.interpolate docstring, see #12918 (comment) and #12918 (comment)

I went to the interpolate function back and forth and I come to the conclusion that we totally should fix this first. When we want to ensure that fillna(method='ffill') and interpolate(method='ffill') behave the same way, why shouldn't we just call fillna inside of interpolate in this case?

The whole interpolate function looks pretty much like things have grown over time and the quick fix is prefered to the generic, clean solution (with transposing back and forth, and so on). I am working on a cleaner solution. Am curious for your feedback.

EDIT:
I found a quick solution which fixes the referenced tickets (#33956 and #12918 and #29146). I am not totally satisfied with this solution since it is adding complexity (though only a little) to an already complex code. I would still suggest that a refactoring of the total interpolate logic would make things much clearer.

pep8speaks · 2020-05-13T21:20:02Z

Hello @CloseChoice! 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-05-29 16:26:02 UTC

…x_33956

consistens with previous version, add TODO

simonjayhawkins

Thanks @CloseChoice for the updates, is it feasible for the fixes for #12918 and #29146 be split off into a pre-cursor PR. smaller PRs with more atomic changes are easier to review/quicker to merge. also needs a whatsnew.

simonjayhawkins · 2020-05-17T14:59:47Z

pandas/tests/frame/methods/test_interpolate.py

@@ -296,3 +297,45 @@ def test_interp_string_axis(self, axis_name, axis_number):
        result = df.interpolate(method="linear", axis=axis_name)
        expected = df.interpolate(method="linear", axis=axis_number)
        tm.assert_frame_equal(result, expected)
+
+    @pytest.mark.parametrize("axis", [0, 1])
+    def test_interp_ffill(self, axis):


can you parameterise these tests @pytest.mark.parametrize("method", ["ffill", "bfill", "pad"])

simonjayhawkins · 2020-05-17T15:03:48Z

pandas/core/generic.py

+                "Only `method=linear` interpolation is supported on MultiIndexes."
+            )
+
+        if method in ["backfill", "bfill", "pad", "ffill"]:


can you add a comment here that for these methods, limit_direction and limit_area are being ignored and include a link to #26796

CloseChoice · 2020-05-17T15:53:55Z

Thanks @CloseChoice for the updates, is it feasible for the fixes for #12918 and #29146 be split off into a pre-cursor PR. smaller PRs with more atomic changes are easier to review/quicker to merge. also needs a whatsnew.

Thanks for your review. Actually fixing #12918 and #29146 has the side effect of fixing #33956, since the fillna-methods don't have the issue described in #33956. So I don't see possibility to split this into two PRs though I totally agree with you to have small, atomic PRs.

jreback · 2020-05-18T13:22:07Z

pandas/core/generic.py


+        # todo: change interpolation so that no transposing is necessary


jreback · 2020-05-18T13:23:29Z

pandas/tests/frame/methods/test_interpolate.py

@@ -202,7 +202,8 @@ def test_interp_leading_nans(self, check_scipy):
            result = df.interpolate(method="polynomial", order=1)
            tm.assert_frame_equal(result, expected)

-    def test_interp_raise_on_only_mixed(self):
+    @pytest.mark.parametrize("axis", [0, 1])


use the axis fixture (e.g. just pass axis as the arg)

jreback · 2020-05-18T13:23:40Z

pandas/tests/frame/methods/test_interpolate.py

@@ -213,7 +214,7 @@ def test_interp_raise_on_only_mixed(self):
            }
        )
        with pytest.raises(TypeError):
-            df.interpolate(axis=1)
+            df.astype("object").interpolate(axis=axis)


why is this changed?

check the error message here

jreback · 2020-05-18T13:23:58Z

pandas/tests/frame/methods/test_interpolate.py

@@ -296,3 +297,18 @@ def test_interp_string_axis(self, axis_name, axis_number):
        result = df.interpolate(method="linear", axis=axis_name)
        expected = df.interpolate(method="linear", axis=axis_number)
        tm.assert_frame_equal(result, expected)
+
+    @pytest.mark.parametrize("method", ["ffill", "bfill", "pad"])
+    @pytest.mark.parametrize("axis", [0, 1])


axis fixture

jreback · 2020-05-18T13:28:21Z

pandas/core/generic.py

+            )
+
+        # for the methods backfill, bfill, pad, ffill limit_direction and limit_area
+        # are being ignored, see #26796 for more information


list as gh-26796

pandas/tests/frame/methods/test_interpolate.py

doc/source/whatsnew/v1.1.0.rst

…nterpolate and check message

jreback · 2020-06-02T22:42:56Z

thanks @CloseChoice very nice

simonjayhawkins requested changes May 12, 2020

View reviewed changes

jreback added Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate Bug labels May 12, 2020

jreback added this to the 1.1 milestone May 12, 2020

CloseChoice added 2 commits May 13, 2020 22:36

all tests working

5ef4eba

add tests

4f3bcbc

formatting with black pandas

fda8be0

CloseChoice force-pushed the fix_33956 branch from 16d17b1 to fda8be0 Compare May 13, 2020 21:28

CloseChoice added 2 commits May 13, 2020 23:46

Merge branch 'master' of https://github.com/pandas-dev/pandas into fi…

94a738f

…x_33956

change order of raising TypeError and calling fillna functions to be

5b21ba5

consistens with previous version, add TODO

CloseChoice requested a review from simonjayhawkins May 14, 2020 16:36

fix line length for linting

1280b32

simonjayhawkins reviewed May 17, 2020

View reviewed changes

add comments for fillna methods and use parametrize for tests

01fec55

added whatsnew entry

baeae0c

CloseChoice requested a review from simonjayhawkins May 17, 2020 21:41

jreback requested changes May 18, 2020

View reviewed changes

pandas/tests/frame/methods/test_interpolate.py Outdated Show resolved Hide resolved

doc/source/whatsnew/v1.1.0.rst Outdated Show resolved Hide resolved

update whatsnew, add PR comment to generic.py, use fixtures in test_i…

502f827

…nterpolate and check message

CloseChoice requested a review from jreback May 27, 2020 22:18

new line in v1.1.0.rst

42895db

jreback approved these changes Jun 2, 2020

View reviewed changes

jreback merged commit b405904 into pandas-dev:master Jun 2, 2020

simonjayhawkins mentioned this pull request Jun 12, 2020

FIX: fix interpolate with kwarg limit area and limit direction using pad or bfill #31048

Closed

5 tasks

simonjayhawkins mentioned this pull request Jun 13, 2020

REF: refactor NDFrame.interpolate to avoid dispatching to fillna #34752

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix bfill, ffill and pad when calling with df.interpolate with column… #33959

fix bfill, ffill and pad when calling with df.interpolate with column… #33959

CloseChoice commented May 3, 2020 •

edited

Loading

simonjayhawkins left a comment

simonjayhawkins commented May 12, 2020

jreback commented May 12, 2020

CloseChoice commented May 13, 2020 •

edited

Loading

pep8speaks commented May 13, 2020 •

edited

Loading

simonjayhawkins left a comment

simonjayhawkins May 17, 2020

CloseChoice May 17, 2020

simonjayhawkins May 17, 2020

CloseChoice May 17, 2020

CloseChoice commented May 17, 2020

jreback May 18, 2020

jreback May 18, 2020

jreback May 18, 2020

jreback May 18, 2020

CloseChoice Jun 2, 2020

jreback May 18, 2020

jreback May 18, 2020

jreback commented Jun 2, 2020


		# todo: change interpolation so that no transposing is necessary

fix bfill, ffill and pad when calling with df.interpolate with column… #33959

fix bfill, ffill and pad when calling with df.interpolate with column… #33959

Conversation

CloseChoice commented May 3, 2020 • edited Loading

simonjayhawkins left a comment

Choose a reason for hiding this comment

simonjayhawkins commented May 12, 2020

jreback commented May 12, 2020

CloseChoice commented May 13, 2020 • edited Loading

pep8speaks commented May 13, 2020 • edited Loading

Comment last updated at 2020-05-29 16:26:02 UTC

simonjayhawkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CloseChoice commented May 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Jun 2, 2020

CloseChoice commented May 3, 2020 •

edited

Loading

CloseChoice commented May 13, 2020 •

edited

Loading

pep8speaks commented May 13, 2020 •

edited

Loading