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

sanggon6107 · 2025-01-23T11:18:01Z

closes BUG: Series.mask incorrectly replaces positions of pd.NA in the cond argument #60729 (Replace xxxx with the GitHub issue number)
Tests added and passed if fixing a bug or adding a new feature
All code checks passed.
Added type annotations to new arguments/methods/functions.
Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

Added fillna at the beginning of _where so that we can fill pd.NA.
Since this is my first PR, please correct me if I'm mistaken. Thanks!

…here. (pandas-dev#60729)

pandas/tests/series/indexing/test_mask.py

WillAyd · 2025-01-23T17:04:48Z

pandas/core/generic.py

 if axis is not None:
 axis = self._get_axis_number(axis)

+ # We should not be filling NA. See GH#60729


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)

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?

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 :

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()

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(),

)

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?

@WillAyd

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.

By filling in the missing values on cond with True, the missing value in the caller propagates. It's not filling in this missing values on cond that then fails to properly propagate the caller's missing value.

Co-authored-by: WillAyd <will_ayd@innobi.io>

sanggon6107 · 2025-02-03T12:42:16Z

FYI, it seems this has already been discussed at #53124 (comment)

doc/source/whatsnew/v3.0.0.rst

Co-authored-by: Xiao Yuan <yuanx749@gmail.com>

rhshadrach

I think this looks like the right approach, but a question on if we can simplify here.

rhshadrach · 2025-03-01T12:56:35Z

pandas/core/generic.py

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


We also do fillna on L9704 below. Can these be combined?

Also, what other types besides ndarray and NDFrame get here?

Also, what other types besides ndarray and NDFrame get here?

Hi @rhshadrach, thanks for the review!
I've tried to find if there are any other types that possibly can get here, but I couldn't find any.

According to the documentation, cond should be one of these : bool Series/DataFrame, array-like, or callable.
And array-like such as list/tuple would be converted to NDFrame/np.ndarray via below codes.

In case we input list/tuple to mask():
or In case we input callable(a function that returns list or tuple) to mask():
(pandas/core/generic.py > NDFrame.mask())

pandas/pandas/core/generic.py

Lines 10096 to 10098 in 57fd502

# see gh-21891

if not hasattr(cond, "__invert__"):

cond = np.array(cond)

In case we input list/tuple to where():
or In case we input scalar to 'mask()' or 'where():
or In case we input callable(a function that returns list or tuple) to where():
(pandas/core/generic.py > NDFrame._where())

pandas/pandas/core/generic.py

Lines 9712 to 9717 in 57fd502

else:

if not hasattr(cond, "shape"):

cond = np.asanyarray(cond)

if cond.shape != self.shape:

raise ValueError("Array conditional must be same shape as self")

cond = self._constructor(cond, **self._construct_axes_dict(), copy=False)

Please let me know if I'm missing anything.

Hi @rhshadrach, sorry for the confusion. I just realized that cond could be either list or tuple when we input a callable to where(). Will revise the code.

@rhshadrach,
I've tried to combine this code with fillna(inplace) at L9732 as you said, but it seems this would result in some test failures since align() at L9722 sometimes returns an ndarray with full of np.nan, and then cond is supposed to be filled with inplace(=False) by L9732. And several tests is current expecting this behaviour as it is. For example, at tests.frame.indexing.test_mask.test_mask_stringdtype[Series] :

tests.frame.indexing.test_mask.test_mask_stringdtype[Series]

def test_mask_stringdtype(frame_or_series): # GH 40824 obj = DataFrame( {"A": ["foo", "bar", "baz", NA]}, index=["id1", "id2", "id3", "id4"], dtype=StringDtype(), ) filtered_obj = DataFrame( {"A": ["this", "that"]}, index=["id2", "id3"], dtype=StringDtype() ) expected = DataFrame( {"A": [NA, "this", "that", NA]}, index=["id1", "id2", "id3", "id4"], dtype=StringDtype(), ) if frame_or_series is Series: obj = obj["A"] filtered_obj = filtered_obj["A"] expected = expected["A"] filter_ser = Series([False, True, True, False]) result = obj.mask(filter_ser, filtered_obj) tm.assert_equal(result, expected)

result >>> id1 foo id2 bar id3 baz id4 <NA> Name: A, dtype: string expected >>> id1 <NA> id2 this id3 that id4 <NA> Name: A, dtype: string

I suspect the behaviour of align() at L9722 is not desirable because current code will re-initialize the cond for these cases and fill cond with False regardless of cond given by users as below.

obj >>> id1 foo id2 bar id3 baz id4 <NA> Name: A, dtype: string filtered_obj >>> id2 this id3 that Name: A, dtype: string filter_ser = pd.Series([False, True, True, False]) filter_ser_2 = pd.Series([False, False, False, False]) filter_ser_3 = pd.Series([True, True, True, True]) result = obj.mask(filter_ser, filtered_obj) # Should return ["foo", "this", "that", pd.NA]. But this test is currently expecthing to be [pd.NA, "this", "that", pd.NA] result >>> id1 <NA> id2 this id3 that id4 <NA> Name: A, dtype: string result_2 = obj.mask(filter_ser_2, filtered_obj) # Should return ["foo", "bar", "baz", pd.NA] result_2 >>> id1 <NA> id2 this id3 that id4 <NA> Name: A, dtype: string result_3 = obj.mask(filter_ser_3, filtered_obj) # Should reutrn ["pd.NA, "this", "that", pd.NA] result_3 >>> id1 <NA> id2 this id3 that id4 <NA> Name: A, dtype: string

I think I'd better open another issue regarding this, but for now, I suppose we'd best to leave fillna() as it is, not combining with the below one. Could you please let me know what you think about this?

failing tests

tests.frame.indexing.test_getitem.TestGetitemBooleanMask.test_getitem_boolean_series_with_duplicate_columns
tests.frame.indexing.test_indexing.TestDataFrameIndexing.test_setitem_cast
tests.frame.indexing.test_mask.test_mask_stringdtype[Series]
tests.frame.indexing.test_mask.test_mask_where_dtype_timedelta
tests.frame.indexing.test_where.test_where_bool_comparison
tests.frame.indexing.test_where.test_where_string_dtype[Series]
tests.frame.indexing.test_where.TestDataFrameIndexingWhere.test_where_alignment[float_string]
tests.frame.indexing.test_where.TestDataFrameIndexingWhere.test_where_alignment[mixed_int]
tests.frame.indexing.test_where.TestDataFrameIndexingWhere.test_where_bug
tests.frame.indexing.test_where.TestDataFrameIndexingWhere.test_where_invalid
tests.frame.indexing.test_where.TestDataFrameIndexingWhere.test_where_ndframe_align
tests.frame.indexing.test_where.TestDataFrameIndexingWhere.test_where_none
tests.indexing.multiindex.test_setitem.TestMultiIndexSetItem.test_frame_getitem_setitem_multislice
tests.indexing.test_indexing.TestMisc.test_no_reference_cycle
tests.series.indexing.test_mask.test_mask_casts
tests.series.indexing.test_where.test_where_error
tests.series.indexing.test_where.test_where_setitem_invalid

rhshadrach · 2025-03-01T12:57:27Z

pandas/tests/series/indexing/test_mask.py

+
+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())


Can you also add a test for arrow. Can parametrize with e.g.

@pytest.mark.parametrize("dtype", ["Int64", "int64[pyarrow]")

Thanks, just added a test for pyarrow.

Co-authored by: rhshadrach <rhshadrach@gmail.com>

rhshadrach

Looking good!

rhshadrach · 2025-03-04T01:11:14Z

pandas/core/generic.py

+ cond[isna(cond)] = True
+ elif isinstance(cond, NDFrame):
+ cond = cond.fillna(True)
+ elif isinstance(cond, (list, tuple)):


The docs for where state that cond can be "list-like", so we should be using is_list_like instead of this condition. However, can you instead move this section so that it's combined with the if block on L9714 immediately below.

I've change the code, but it seems this causes test failures. I'll convert the status to draft and revise the code.

just confirmed that all the tests passed. Thanks!

rhshadrach · 2025-03-04T01:12:03Z

pandas/tests/series/indexing/test_mask.py

+from pandas import (
+ Series,
+)


can you revert this change.

Code reverted. Thanks!

rhshadrach · 2025-03-04T01:13:27Z

pandas/tests/series/indexing/test_mask.py

+ if dtype == "int64[pyarrow]":
+ pytest.importorskip("pyarrow")


Instead, can you change the parametrization to be:

pytest.param("int64[pyarrow]", marks=td.skip_if_no("pyarrow"))

Thanks for the suggestion. just changed the code.

sanggon6107 · 2025-03-05T14:22:29Z

Many thanks for your feedback, @rhshadrach . All the changes you requested are now reflected. Could you review the changes?

mroeschke · 2025-03-05T18:28:32Z

pandas/core/generic.py

 if not hasattr(cond, "shape"):
 cond = np.asanyarray(cond)
+ else:
+ cond = np.array(cond)


Suggested change

cond = np.array(cond)

cond = extract_array(cond, extract_numpy=True)

Could we avoid the copy here somehow?

I think we might be able to call fillna(True) right below the self._constructor() since cond will become an NDFrame there.

Hi @mroeschke ,confirmed that all the checks passed. Could you review the code change?

Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>

sanggon6107 · 2025-03-06T01:28:17Z

Hi @mroeschke , Thanks for the suggestion! But it seems some tests fail when we changed the code np.array() to extract_array(cond, extract_numpy=True). I'll revert the code for now and see if there is still room for improvement.

rhshadrach · 2025-04-27T13:01:51Z

pandas/core/generic.py

 # align the cond to same shape as myself
 cond = common.apply_if_callable(cond, self)
 if isinstance(cond, NDFrame):
+ cond = cond.fillna(True)


Yea, that makes sense to me. But I believe that's only necessary to fix the bug mentioned above and not necessarily for this PR. If that is the case, I think it should be handled separately.

rhshadrach · 2025-04-27T13:25:09Z

pandas/core/generic.py

+ if all(
+ x is NA or isinstance(x, (np.bool_, bool)) or x is np.nan
+ for x in cond.flatten()
+ ):


I'm not following why cond = cond.astype(bool) would be incorrect here. bool(np.nan) gives True, and so while users may be surprised by pandas replacing np.nan in such a case, I believe it is technically correct. pd.NA on the other hand doesn't allow conversion to Boolean and propagates in comparisons (unlike nan), so I think we should special case this.

jbrockmendel · 2025-07-08T17:12:26Z

pandas/core/generic.py

 else:
 if not hasattr(cond, "shape"):
- cond = np.asanyarray(cond)
+ cond = np.asanyarray(cond, dtype=object)


no way to avoid this?

jbrockmendel · 2025-07-08T17:12:37Z

pandas/core/generic.py

 # see gh-21891
 if not hasattr(cond, "__invert__"):
- cond = np.array(cond)
+ cond = np.array(cond, dtype=object)


again, no way to avoid this?

jbrockmendel · 2025-07-08T17:12:48Z

pandas/core/generic.py

+
+ if isinstance(cond, np.ndarray):
+ if all(
+ x is NA or isinstance(x, (np.bool_, bool)) or x is np.nan


lib.is_bool for the isinstance checks

jbrockmendel · 2025-11-03T16:07:37Z

My intution here is that we want ser.mask(cond, -99) to behave like ser[conds] = -99 (but with a copy). For that, non-bool conds don't really make sense. i think we only want to fill pd.NA specifically in boolean/bool[pyarrow] dtypes, not float/object?

sanggon6107 · 2025-11-20T13:56:12Z

Sorry for the late response, @jbrockmendel . Do you mean we have to raise when cond has nan or NA then?

jbrockmendel · 2025-12-02T16:21:34Z

Do you mean we have to raise when cond has nan or NA then?

That's what I would do, yes

… into add-mask-fillna

sanggon6107 added 4 commits January 23, 2025 19:14

BUG: Add fillna so that cond doesnt contain NA at the beginning of _w…

bc9a942

…here. (pandas-dev#60729)

TST: Add tests for mask with NA. (pandas-dev#60729)

558569f

BUG: Fix _where to make np.ndarray mutable. (pandas-dev#60729)

bbbc720

DOC: Add documentation regarding the bug (pandas-dev#60729)

e2f32cb

WillAyd requested changes Jan 23, 2025

View reviewed changes

sanggon6107 and others added 3 commits January 25, 2025 17:01

Merge branch 'main' into add-mask-fillna

6fd8986

ENH: Optimze test_mask_na()

d2d5f62

Co-authored-by: WillAyd <will_ayd@innobi.io>

BUG: Fix a bug in test_mask_na() (pandas-dev#60729)

475f2d1

sanggon6107 requested a review from WillAyd February 3, 2025 12:43

yuanx749 reviewed Feb 8, 2025

View reviewed changes

doc/source/whatsnew/v3.0.0.rst Outdated Show resolved Hide resolved

Update doc/source/whatsnew/v3.0.0.rst

db30b58

Co-authored-by: Xiao Yuan <yuanx749@gmail.com>

rhshadrach requested changes Mar 1, 2025

View reviewed changes

sanggon6107 added 5 commits March 3, 2025 15:48

Merge branch 'main' into add-mask-fillna

55fe420

Add test arguments for test_mask_na

cb94cf7

Co-authored by: rhshadrach <rhshadrach@gmail.com>

Fix whatsnew

71e442e

Fix test failures by adding importorskip

b6bd3af

Fill True when tuple or list cond has np.nan/pd.NA

8bac997

rhshadrach requested changes Mar 4, 2025

View reviewed changes

sanggon6107 added 3 commits March 4, 2025 22:46

Merge branch 'main' into add-mask-fillna

89bc1b4

Optimize _where

f154cf5

Optimize test_mask_na

eed6121

sanggon6107 marked this pull request as draft March 4, 2025 16:22

sanggon6107 and others added 2 commits March 5, 2025 19:43

Add np.array for read-only ndarray

9ac81f0

Merge branch 'main' into add-mask-fillna

7e3fd3a

sanggon6107 marked this pull request as ready for review March 5, 2025 14:17

mroeschke reviewed Mar 5, 2025

View reviewed changes

Update generic.py

8c5ffff

Co-authored-by: Matthew Roeschke <10647082+mroeschke@users.noreply.github.com>

sanggon6107 added 3 commits April 20, 2025 19:52

Merge branch 'main' into add-mask-fillna

88b0530

fillna after constructor

044d0a9

Parametrize cond type

601f6c9

rhshadrach requested changes Apr 27, 2025

View reviewed changes

sanggon6107 added 4 commits May 13, 2025 21:23

Merge branch 'main' into add-mask-fillna

4528a54

Merge branch 'main' into add-mask-fillna

acf805a

Merge branch 'main' into add-mask-fillna

3239d2f

Merge branch 'main' into add-mask-fillna

f7a1e5a

jbrockmendel reviewed Jul 8, 2025

View reviewed changes

This was referenced Jul 10, 2025

BUG: mask in test_mask_stringdtype would always return the same result regardless of cond #61824

Closed

DOC: Typo within Series.mask() docs - alignment is done between self and cond, not cond and other #61781

Open

Nadav-Zilberberg mentioned this pull request Jul 15, 2025

BUG: mask in test_mask_stringdtype would always return the same result regardless of cond Nadav-Zilberberg/pandas-test#15

Closed

sanggon6107 added 10 commits July 15, 2025 22:52

Merge main

f3ed54d

Replace isinstance with is_bool

16dda53

Merge branch 'main' into add-mask-fillna

dfa96ac

Remove dtype

e557c05

handle list with nan

7e0d4e0

Merge branch 'main' into add-mask-fillna

85f35e9

Use astype instead of replace

e2df2d0

Merge branch 'main' into add-mask-fillna

0526b02

Merge branch 'main' into add-mask-fillna

ffd15b7

Merge branch 'main' into add-mask-fillna

17b83cb

Merge branch 'main' into add-mask-fillna

350b75d

sanggon6107 added 2 commits December 11, 2025 21:08

Resolve git conflict

e7f8fa7

Merge branch 'add-mask-fillna' of https://github.com/sanggon6107/pandas…

506f39d

… into add-mask-fillna

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

	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(),
	)

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

	cond = extract_bool_array(cond)

	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

	# see gh-21891
	if not hasattr(cond, "__invert__"):
	cond = np.array(cond)

	else:
	if not hasattr(cond, "shape"):
	cond = np.asanyarray(cond)
	if cond.shape != self.shape:
	raise ValueError("Array conditional must be same shape as self")
	cond = self._constructor(cond, **self._construct_axes_dict(), copy=False)

	cond = np.array(cond)
	cond = extract_array(cond, extract_numpy=True)

Uh oh!

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

Are you sure you want to change the base?

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

Conversation

sanggon6107 commented Jan 23, 2025

Uh oh!

Choose a reason for hiding this comment

sanggon6107 Jan 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhshadrach Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

sanggon6107 commented Feb 3, 2025

Uh oh!

rhshadrach 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

tests.frame.indexing.test_mask.test_mask_stringdtype[Series]

failing tests

Choose a reason for hiding this comment

sanggon6107 Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

rhshadrach left a comment

Choose a reason for hiding this comment

rhshadrach Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

sanggon6107 Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanggon6107 Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanggon6107 commented Mar 5, 2025

mroeschke Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

sanggon6107 commented Mar 6, 2025

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

jbrockmendel commented Nov 3, 2025

sanggon6107 commented Nov 20, 2025

jbrockmendel commented Dec 2, 2025

Labels

6 participants

sanggon6107 Jan 25, 2025 •

edited

Loading

rhshadrach Mar 1, 2025 •

edited

Loading

sanggon6107 Mar 3, 2025 •

edited

Loading

rhshadrach Mar 4, 2025 •

edited

Loading

sanggon6107 Mar 4, 2025 •

edited

Loading

sanggon6107 Mar 4, 2025 •

edited

Loading

mroeschke Mar 5, 2025 •

edited

Loading