Skip to content
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

BUG: Add fillna at the beginning of _where not to fill NA. #60729 #60772

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

BUG: Add fillna at the beginning of _where not to fill NA. #60729 #60772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bc9a942
BUG: Add fillna so that cond doesnt contain NA at the beginning of _w…
sanggon6107 Jan 23, 2025
558569f
TST: Add tests for mask with NA. (#60729)
sanggon6107 Jan 23, 2025
bbbc720
BUG: Fix _where to make np.ndarray mutable. (#60729)
sanggon6107 Jan 23, 2025
e2f32cb
DOC: Add documentation regarding the bug (#60729)
sanggon6107 Jan 23, 2025
6fd8986
Merge branch 'main' into add-mask-fillna
sanggon6107 Jan 25, 2025
d2d5f62
ENH: Optimze test_mask_na()
sanggon6107 Jan 25, 2025
475f2d1
BUG: Fix a bug in test_mask_na() (#60729)
sanggon6107 Jan 25, 2025
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
1 change: 1 addition & 0 deletions doc/source/whatsnew/v3.0.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,7 @@ Interval

Indexing
^^^^^^^^
- Bug in :func:`Series.mask` unexpectedly filling pd.NA (:issue:`60729`)
- Bug in :meth:`DataFrame.__getitem__` returning modified columns when called with ``slice`` in Python 3.12 (:issue:`57500`)
- Bug in :meth:`DataFrame.from_records` throwing a ``ValueError`` when passed an empty list in ``index`` (:issue:`58594`)
- Bug in :meth:`MultiIndex.insert` when a new value inserted to a datetime-like level gets cast to ``NaT`` and fails indexing (:issue:`60388`)
Expand Down
7 changes: 7 additions & 0 deletions pandas/core/generic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9674,6 +9674,13 @@ def _where(
if axis is not None:
axis = self._get_axis_number(axis)

# We should not be filling NA. See GH#60729
Copy link
Member

Choose a reason for hiding this comment

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

Is this trying to fill missing values when NaN is the missing value indicator? I don't think that is right either - the missing values should propogate for all types. We may just be missing coverage for the NaN case (which should be added to the test)

Copy link
Author

@sanggon6107 sanggon6107 Jan 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the feedback, @WillAyd .
I thought we could make the values propagate by filling cond with True, since _where() would finally keep the values in self alive where its cond is True.
Even if I don't fill those values here, _where would call fillna() using inplace at the below code. That's also why the result varies depending on whether inpalce=True or not.

pandas/pandas/core/generic.py

Lines 9695 to 9698 in e3b2de8

# make sure we are boolean
fill_value = bool(inplace)
cond = cond.fillna(fill_value)
cond = cond.infer_objects()

Could you explain in more detail what you mean by propagate for all type? Do you mean we need to keep NA as it is even after this line?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @WillAyd,

I've done some further investigations on this, but I still belive the current code is the simplest way to make the missing values propagate.
If we want to let NA propagate without calling fillna() here, there might be too many code changes needed. See below codes :

  1. Need to change the below code so that we don't fill the missing values when caller is where() or mask(). If we don't, fillna() will fill them with inplace.

pandas/pandas/core/generic.py

Lines 9695 to 9698 in f1441b2

# make sure we are boolean
fill_value = bool(inplace)
cond = cond.fillna(fill_value)
cond = cond.infer_objects()

  1. Need to change the below code as well since to_numpy() will fill the missing value using inplace when cond is a DataFrame.

pandas/pandas/core/generic.py

Lines 9703 to 9716 in f1441b2

if not isinstance(cond, ABCDataFrame):
# This is a single-dimensional object.
if not is_bool_dtype(cond):
raise TypeError(msg.format(dtype=cond.dtype))
else:
for _dt in cond.dtypes:
if not is_bool_dtype(_dt):
raise TypeError(msg.format(dtype=_dt))
if cond._mgr.any_extension_types:
# GH51574: avoid object ndarray conversion later on
cond = cond._constructor(
cond.to_numpy(dtype=bool, na_value=fill_value),
**cond._construct_axes_dict(),
)

  1. Since extract_bool_array() fills the missing values using arg na_value=False at EABackedBlock.where(), we might need to find every single NA index from cond before we call this function(using isna() for example) and then implement additional behaviour to make those values propagate at ExtensionArray._where().

pandas/pandas/core/internals/blocks.py

Lines 1664 to 1668 in f1441b2

def where(self, other, cond) -> list[Block]:
arr = self.values.T
cond = extract_bool_array(cond)

pandas/pandas/core/array_algos/putmask.py

Lines 116 to 127 in f1441b2

def extract_bool_array(mask: ArrayLike) -> npt.NDArray[np.bool_]:
"""
If we have a SparseArray or BooleanArray, convert it to ndarray[bool].
"""
if isinstance(mask, ExtensionArray):
# We could have BooleanArray, Sparse[bool], ...
# Except for BooleanArray, this is equivalent to just
# np.asarray(mask, dtype=bool)
mask = mask.to_numpy(dtype=bool, na_value=False)
mask = np.asarray(mask, dtype=bool)
return mask

If _where() is trying to fill the missing values for cond anyway, I think we don't necessarily have to disfavour the current code change. Could you give me some feedback?

if isinstance(cond, np.ndarray):
cond = np.array(cond)
cond[np.isnan(cond)] = True
elif isinstance(cond, NDFrame):
cond = cond.fillna(True)

# align the cond to same shape as myself
cond = common.apply_if_callable(cond, self)
if isinstance(cond, NDFrame):
Expand Down
14 changes: 13 additions & 1 deletion pandas/tests/series/indexing/test_mask.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import numpy as np
import pytest

from pandas import Series
from pandas import (
Int64Dtype,
Series,
)
import pandas._testing as tm


Expand Down Expand Up @@ -67,3 +70,12 @@ def test_mask_inplace():
rs = s.copy()
rs.mask(cond, -s, inplace=True)
tm.assert_series_equal(rs, s.mask(cond, -s))


def test_mask_na():
# We should not be filling pd.NA. See GH#60729
series = Series([None, 1, 2, None, 3, 4, None], dtype=Int64Dtype())
result = series.mask(series <= 2, -99)
expected = Series([None, -99, -99, None, 3, 4, None], dtype=Int64Dtype())

tm.assert_series_equal(result, expected)