BUG: Fix for fillna ignoring axis=1 parameter (issues #17399 #17409) #28441

jorvis · 2019-09-14T02:43:19Z

closes fillna ignoring axis=1 parameter #17399 BUG: fillna ignoring axis=1 parameter. #17399 #17409
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2019-09-14T02:43:24Z

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

In the file pandas/tests/frame/test_missing.py:

Line 675:9: E265 block comment should start with '# '
Line 677:20: E231 missing whitespace after ','
Line 677:22: E231 missing whitespace after ','
Line 677:24: E231 missing whitespace after ','
Line 678:20: E231 missing whitespace after ','
Line 678:27: E231 missing whitespace after ','
Line 678:29: E231 missing whitespace after ','
Line 679:20: E231 missing whitespace after ','
Line 679:23: E231 missing whitespace after ','
Line 679:26: E231 missing whitespace after ','
Line 682:20: E231 missing whitespace after ','
Line 682:22: E231 missing whitespace after ','
Line 682:24: E231 missing whitespace after ','
Line 683:20: E231 missing whitespace after ','
Line 683:22: E231 missing whitespace after ','
Line 683:24: E231 missing whitespace after ','
Line 684:20: E231 missing whitespace after ','
Line 684:23: E231 missing whitespace after ','
Line 684:26: E231 missing whitespace after ','

Comment last updated at 2019-09-19 21:56:54 UTC

jorvis · 2019-09-14T02:49:56Z

Here's an example of this issue in Jupyter:

https://imgur.com/a/3ZrXqSv

gfyoung · 2019-09-15T03:46:46Z

pandas/core/internals/blocks.py

@@ -386,7 +386,7 @@ def apply(self, func, **kwargs):

        return result

-    def fillna(self, value, limit=None, inplace=False, downcast=None):
+    def fillna(self, value, axis=0, limit=None, inplace=False, downcast=None):


As is, this is a breaking API change. To be safe, you would need to put axis at the end.

Can you also add an annotation for axis? Should be importable from pandas._typing

As is, this is a breaking API change. To be safe, you would need to put axis at the end.

I have just done this, thanks.

Can you also add an annotation for axis? Should be importable from pandas._typing

Sorry, I'm not sure what that means. Could you direct me to an example or docs?

Yea so a variable annotation. You'd essentially do:

from pandas._typing import Axis def fillna(..., axis: Axis = 0):

OK, thanks. I have pushed this.

WillAyd

Can you add a whatsnew for v1.0.0?

WillAyd · 2019-09-16T01:55:02Z

pandas/tests/frame/test_missing.py

@@ -671,6 +671,14 @@ def test_fillna_columns(self):
        expected = df.astype(float).fillna(method="ffill", axis=1)
        assert_frame_equal(result, expected)

+    def test_fillna_columns(self):
+        df = DataFrame(np.random.randn(10, 4))


Can you add GH 17399 as a comment here?

Also generally for test you can simplify by just constructing a DataFrame with literal values instead of random data - should make smaller and help readability

I've added the comment (and properly fixed the test function name). I'm afraid I found this bug on the first day of my learning to use pandas, so I'm not quite there yet to write a custom test. Rather, I used/modified the existing one from test_fillna_columns()

Hmm this test works on master for me without these changes - is this capturing the changed behavior?

@jorvis can you respond here? Need to make sure we're actually fixing a real issue.

I'll try to make a more direct test case. This is the demo that made me find it:

https://imgur.com/a/3ZrXqSv

After that I found an existing PR which had grown stale, and @jreback asked me to update it, so I thought I'd give it a try.

I have updated it now to reflect a test similar to that Jupyter image above.

WillAyd · 2019-09-16T01:56:30Z

pandas/core/internals/blocks.py

@@ -386,7 +386,7 @@ def apply(self, func, **kwargs):

        return result

-    def fillna(self, value, limit=None, inplace=False, downcast=None):
+    def fillna(self, value, axis=0, limit=None, inplace=False, downcast=None):


Can you also add an annotation for axis? Should be importable from pandas._typing

…test_fillna_rows(), added comment to reference bug

pandas/core/generic.py

WillAyd · 2019-09-16T16:02:18Z

pandas/tests/frame/test_missing.py

@@ -671,6 +671,14 @@ def test_fillna_columns(self):
        expected = df.astype(float).fillna(method="ffill", axis=1)
        assert_frame_equal(result, expected)

+    def test_fillna_columns(self):
+        df = DataFrame(np.random.randn(10, 4))


Hmm this test works on master for me without these changes - is this capturing the changed behavior?

TomAugspurger · 2019-09-16T21:52:28Z

doc/source/whatsnew/v1.0.0.rst

@@ -233,7 +233,7 @@ Other
 - Trying to set the ``display.precision``, ``display.max_rows`` or ``display.max_columns`` using :meth:`set_option` to anything but a ``None`` or a positive int will raise a ``ValueError`` (:issue:`23348`)
 - Using :meth:`DataFrame.replace` with overlapping keys in a nested dictionary will no longer raise, now matching the behavior of a flat dictionary (:issue:`27660`)
 - :meth:`DataFrame.to_csv` and :meth:`Series.to_csv` now support dicts as ``compression`` argument with key ``'method'`` being the compression method and others as additional compression options when the compression method is ``'zip'``. (:issue:`26023`)
-
+- :meth:`DataFrame.fillna` when using axis=1 previously failed to replace NaN values (:issue:`17399` and :issue:`17409`)


Is this true in general, or was it just under specific conditions?

I'll try to make a more direct test case. This is the demo that made me find it:

https://imgur.com/a/3ZrXqSv

After that I found an existing PR which had grown stale, and @jreback asked me to update it, so I thought I'd give it a try.

pandas/core/generic.py

pandas/core/internals/blocks.py

TomAugspurger · 2019-09-19T21:20:06Z

pandas/tests/frame/test_missing.py

@@ -671,6 +671,14 @@ def test_fillna_columns(self):
        expected = df.astype(float).fillna(method="ffill", axis=1)
        assert_frame_equal(result, expected)

+    def test_fillna_columns(self):
+        df = DataFrame(np.random.randn(10, 4))


@jorvis can you respond here? Need to make sure we're actually fixing a real issue.

TomAugspurger · 2019-09-19T22:30:21Z

pandas/tests/frame/test_missing.py

+            "b": [5,6,7,8],
+            "c": [9,10,11,6]})
+
+        result = df.fillna(df.mean(axis=1))


I don't see axis passed to fillna here? You pass it to mean but not fillna.

Maybe we should back up and make sure the behavior I'm seeing is expected then an not a bug. In this notebook I first try to replace NaNs with the column average, then the row average. Doing the first works, but the second silently does nothing.

Setting axis=1 on fillna when using the mean where axis=1 causes an error: Currently only can fill with dict/Series column by column.

It seems like calling fillna on a frame with NaNs which doesn't fill them is an issue.

Yea I also think this test might be missing the point. Can you try getting the issue directly from #17399 to work? I think that is relatively clearly laid out

TomAugspurger · 2019-09-20T02:34:01Z

When you do `DataFrame.fillna(Series)` (with no `axis=1`) the indices are aligned. In your test example, nothing is filled because the index of the Series is the original columns from the DataFrame.

…

On Thu, Sep 19, 2019 at 5:52 PM Joshua Orvis ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pandas/tests/frame/test_missing.py <#28441 (comment)>: > @@ -671,6 +671,21 @@ def test_fillna_columns(self): expected = df.astype(float).fillna(method="ffill", axis=1) assert_frame_equal(result, expected) + def test_fillna_rows(self): + #GH17399 + df = pd.DataFrame({ + "a": [1,2,3,4], + "b": [5,np.nan,7,8], + "c": [9,10,11,np.nan]}) + + expected = pd.DataFrame({ + "a": [1,2,3,4], + "b": [5,6,7,8], + "c": [9,10,11,6]}) + + result = df.fillna(df.mean(axis=1)) Maybe we should back up and make sure the behavior I'm seeing is expected then an not a bug. In this notebook <https://imgur.com/a/3ZrXqSv> I first try to replace NaNs with the column average, then the row average. Doing the first works, but the second silently does nothing. Setting axis=1 on fillna when using the mean where axis=1 causes an error: Currently only can fill with dict/Series column by column. It seems like calling fillna on a frame with NaNs which doesn't fill them is an issue. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#28441?email_source=notifications&email_token=AAKAOIVGDMDSGU4CO7NQQBTQKP7CJA5CNFSM4IWVUUTKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCFLIMWA#discussion_r326414395>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKAOIWKL2FSS47XJ5KLIELQKP7CJANCNFSM4IWVUUTA> .

jorvis · 2019-09-20T02:40:13Z

Can you give an example of the syntax to use to fill using the row means?

WillAyd

Can you merge master and address comment(s)?

WillAyd · 2019-10-11T22:07:46Z

pandas/tests/frame/test_missing.py

+            "b": [5,6,7,8],
+            "c": [9,10,11,6]})
+
+        result = df.fillna(df.mean(axis=1))


Yea I also think this test might be missing the point. Can you try getting the issue directly from #17399 to work? I think that is relatively clearly laid out

WillAyd · 2019-11-07T23:23:52Z

Thanks for the PR @jorvis but I think this has gone stale. If not and you'd like to pick back up just ping and can continue

scottboston · 2020-02-03T14:44:36Z

Question. I reported this bug (#17399) back in 2017, I was going to work on fixing this back then but it seems that pratapvardhan worked on the problem. Again someone else found this issue and worked on it. I am not clear what needs to be done to close this issue. It appears that the last test case missed the mark to address the original problem.

What do we need to close this? Write a new test case?

Fix for fillna ignoring axis=1 parameter (issues pandas-dev#17399 pan…

d9f44ed

…das-dev#17409)

jorvis changed the title ~~Fix for fillna ignoring axis=1 parameter (issues #17399 #17409)~~ BUG: Fix for fillna ignoring axis=1 parameter (issues #17399 #17409) Sep 14, 2019

PEP8 fix - line too long nonsense

0ed4f15

gfyoung added Bug Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate labels Sep 15, 2019

gfyoung reviewed Sep 15, 2019

View reviewed changes

gfyoung requested a review from jreback September 15, 2019 03:47

WillAyd requested changes Sep 16, 2019

View reviewed changes

jorvis added 4 commits September 15, 2019 22:04

Re-ordered arguments so axis was last

6837029

Added entry to whatsnew

38c40d9

Added variable annotation for axis references

7536477

Fixed duplicate function name, renaming 2nd test_fillna_columns() to …

df26386

…test_fillna_rows(), added comment to reference bug

WillAyd reviewed Sep 16, 2019

View reviewed changes

pandas/core/generic.py Outdated Show resolved Hide resolved

WillAyd requested changes Sep 16, 2019

View reviewed changes

Added one more annotation for axis

313e934

TomAugspurger reviewed Sep 19, 2019

View reviewed changes

Updated test to use literal values, use row means

e1863fa

TomAugspurger reviewed Sep 19, 2019

View reviewed changes

WillAyd requested changes Oct 11, 2019

View reviewed changes

WillAyd closed this Nov 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix for fillna ignoring axis=1 parameter (issues #17399 #17409) #28441

BUG: Fix for fillna ignoring axis=1 parameter (issues #17399 #17409) #28441

jorvis commented Sep 14, 2019 •

edited

Loading

pep8speaks commented Sep 14, 2019 •

edited

Loading

jorvis commented Sep 14, 2019

gfyoung Sep 15, 2019 •

edited

Loading

WillAyd Sep 16, 2019

jorvis Sep 16, 2019

jorvis Sep 16, 2019

WillAyd Sep 16, 2019

jorvis Sep 16, 2019

WillAyd left a comment

WillAyd Sep 16, 2019

WillAyd Sep 16, 2019

jorvis Sep 16, 2019

WillAyd Sep 16, 2019

TomAugspurger Sep 19, 2019

jorvis Sep 19, 2019

jorvis Sep 19, 2019

WillAyd Sep 16, 2019

WillAyd Sep 16, 2019

TomAugspurger Sep 16, 2019

jorvis Sep 19, 2019

TomAugspurger Sep 19, 2019

TomAugspurger Sep 19, 2019

jorvis Sep 19, 2019

WillAyd Oct 11, 2019

TomAugspurger commented Sep 20, 2019 via email

jorvis commented Sep 20, 2019

WillAyd left a comment

WillAyd Oct 11, 2019

WillAyd commented Nov 7, 2019

scottboston commented Feb 3, 2020 •

edited

Loading

BUG: Fix for fillna ignoring axis=1 parameter (issues #17399 #17409) #28441

BUG: Fix for fillna ignoring axis=1 parameter (issues #17399 #17409) #28441

Conversation

jorvis commented Sep 14, 2019 • edited Loading

pep8speaks commented Sep 14, 2019 • edited Loading

Comment last updated at 2019-09-19 21:56:54 UTC

jorvis commented Sep 14, 2019

gfyoung Sep 15, 2019 • edited Loading

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

WillAyd 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

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

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

TomAugspurger commented Sep 20, 2019 via email

jorvis commented Sep 20, 2019

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd commented Nov 7, 2019

scottboston commented Feb 3, 2020 • edited Loading

jorvis commented Sep 14, 2019 •

edited

Loading

pep8speaks commented Sep 14, 2019 •

edited

Loading

gfyoung Sep 15, 2019 •

edited

Loading

scottboston commented Feb 3, 2020 •

edited

Loading