Skip to content

BUG : Allow axis = 'columns' with limit and no method to work on fillna. #42856

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Aug 20, 2021
Merged

BUG : Allow axis = 'columns' with limit and no method to work on fillna. #42856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c5a545f
solves issue #40989 by raising NotImplementedError and modifying docs
GYvan Aug 2, 2021
bda00fe
Testing the NotImplementedError created to solve issue #40989
GYvan Aug 2, 2021
4cba8ca
pre-commit error fixed
GYvan Aug 2, 2021
44af021
added modified generic.py
GYvan Aug 9, 2021
3043fdb
added modified test_fillna.py
GYvan Aug 9, 2021
9183503
reverted changes for test_frame.py
GYvan Aug 9, 2021
ceef973
added another test
GYvan Aug 12, 2021
49bdfed
added into bug fixes in whatsnew
GYvan Aug 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion doc/source/whatsnew/v1.4.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ Indexing

Missing
^^^^^^^
-
- Bug in :meth:`DataFrame.fillna` with limit and no method ignores axis='columns' or ``axis = 1`` (:issue:`40989`)
-

MultiIndex
Expand Down
17 changes: 14 additions & 3 deletions pandas/core/generic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6306,10 +6306,21 @@ def fillna(
return result if not inplace else None

elif not is_list_like(value):
new_data = self._mgr.fillna(
value=value, limit=limit, inplace=inplace, downcast=downcast
)
if not self._mgr.is_single_block and axis == 1:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this the same as L6254

Copy link
Contributor Author

@GYvan GYvan Aug 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the code is somehow the same, with minor adjustments. I basically used it to make transposes as the ones done when the method is not None. This is because when the method is valid, both axis = 1 and axis = 0 works correctly with the given limit for Fillna due to the transposition at L6254. Therefore, I tried to do the same kind of transposing so that axis = 1 and axis = 0 would work accordingly for the given value and limit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok any possibiltiy of combing those cases into a similar code block?

Copy link
Contributor Author

@GYvan GYvan Aug 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jreback I tried to combine these cases into a similar code block and this is the code that I came up with for a separate top-level if-else block in fillna:

if not self._mgr.is_single_block and axis == 1:
            if value is None or not is_list_like(value):
                    if inplace and value is None:
                        raise NotImplementedError()
                    
                    result = self.T.fillna(method=method, value=value, limit=limit).T

                    # need to downcast here because of all of the transposes
                    result._mgr = result._mgr.downcast()
                    if value is None:
                        return result
                    new_data = result

I also added the condition for x==0 for the remaining of the elif not is_list_like(value) so that it won't interfere with the above changes with x == 1.
However, due to the additional code block: the code structure is slightly modified because before, the code for fillna was clearly split between the case for when value is None and the case for when value is not None, but this additional code block changes that. This new code block means that there are now two places where we would be "branching" on whether a value is None or not: Firstly, in the new code block, and secondly, in the original if-else block. Therefore, I think the new change might make it more difficult to follow the code compared to the original PR where the change was only made to the code that affects fillna with axis = 1 and value is not None.


result = self.T.fillna(value=value, limit=limit).T

# need to downcast here because of all of the transposes
result._mgr = result._mgr.downcast()

new_data = result
else:

new_data = self._mgr.fillna(
value=value, limit=limit, inplace=inplace, downcast=downcast
)
elif isinstance(value, ABCDataFrame) and self.ndim == 2:

new_data = self.where(self.notna(), value)._data
else:
raise ValueError(f"invalid fill value with a {type(value)}")
Expand Down
54 changes: 54 additions & 0 deletions pandas/tests/frame/methods/test_fillna.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,60 @@ def test_fillna_pos_args_deprecation(self):
expected = DataFrame({"a": [1, 2, 3, 0]}, dtype=float)
tm.assert_frame_equal(result, expected)

def test_fillna_with_columns_and_limit(self):
# GH40989
df = DataFrame(
[
[np.nan, 2, np.nan, 0],
[3, 4, np.nan, 1],
[np.nan, np.nan, np.nan, 5],
[np.nan, 3, np.nan, 4],
],
columns=list("ABCD"),
)
result = df.fillna(axis=1, value=100, limit=1)
result2 = df.fillna(axis=1, value=100, limit=2)

expected = DataFrame(
{
"A": Series([100, 3, 100, 100], dtype="float64"),
"B": [2, 4, np.nan, 3],
"C": [np.nan, 100, np.nan, np.nan],
"D": Series([0, 1, 5, 4], dtype="float64"),
},
index=[0, 1, 2, 3],
)
expected2 = DataFrame(
{
"A": Series([100, 3, 100, 100], dtype="float64"),
"B": Series([2, 4, 100, 3], dtype="float64"),
"C": [100, 100, np.nan, 100],
"D": Series([0, 1, 5, 4], dtype="float64"),
},
index=[0, 1, 2, 3],
)

tm.assert_frame_equal(result, expected)
tm.assert_frame_equal(result2, expected2)

def test_fillna_inplace_with_columns_limit_and_value(self):
# GH40989
df = DataFrame(
[
[np.nan, 2, np.nan, 0],
[3, 4, np.nan, 1],
[np.nan, np.nan, np.nan, 5],
[np.nan, 3, np.nan, 4],
],
columns=list("ABCD"),
)

expected = df.fillna(axis=1, value=100, limit=1)
assert expected is not df

df.fillna(axis=1, value=100, limit=1, inplace=True)
tm.assert_frame_equal(df, expected)


def test_fillna_nonconsolidated_frame():
# https://github.com/pandas-dev/pandas/issues/36495
Expand Down