BUG: Don't raise for NDFrame.mask with nullable boolean #36201

dsaxton · 2020-09-07T19:51:30Z

closes BUG: NA does not propagate through mask and where/ raises AssertionError in 1.1.0rc #35429
closes BUG: indexing with DataFrame with nullable boolean dtype #36395
tests added / passed
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

~~(Need to wait until the v1.1.3 whatsnew gets merged before adding a release note.)~~

This is taking the approach of treating NA as False within NDFrame.mask and NDFrame.where when dealing with a nullable boolean mask.

pandas/core/frame.py

pandas/core/generic.py

dsaxton · 2020-09-07T20:02:12Z

pandas/tests/frame/indexing/test_where.py

@@ -159,7 +159,7 @@ def test_where_set(self, where_frame, float_string_frame):

        def _check_set(df, cond, check_dtypes=True):
            dfi = df.copy()
-            econd = cond.reindex_like(df).fillna(True)
+            econd = cond.reindex_like(df).fillna(False)


This test was broken by no longer setting fill_value = bool(inplace) so had to update

simonjayhawkins · 2020-09-07T20:24:32Z

(Need to wait until the v1.1.3 whatsnew gets merged before adding a release note.)

ok to target 1.1.2 if ready before tagged

dsaxton · 2020-09-07T20:53:50Z

ok to target 1.1.2 if ready before tagged

I can add a v1.1.2 note just in case

…e-boolean

dsaxton · 2020-09-07T22:13:07Z

@simonjayhawkins I think having a GitHub Action for merge conflicts like you mentioned would be super useful, do you know by chance how that could be set up?

jreback

this is an intricate change and not appropriate for 1.1.2

pandas/core/frame.py

pandas/core/generic.py

…e-boolean

simonjayhawkins · 2020-09-09T11:28:52Z

i've added the 1.1.3 milestone back while the change note is still in 1.1.3.rst

…e-boolean

jreback

this patch needs to go in 1.2 as its a core change.

pandas/core/frame.py

jreback · 2020-09-11T12:16:18Z

pandas/core/generic.py

@@ -8835,7 +8834,7 @@ def _where(
            # GH#21947 we have an empty DataFrame/Series, could be object-dtype
            cond = cond.astype(bool)

-        cond = -cond if inplace else cond
+        cond = ~cond if inplace else cond


if you are goin to change this then let's clean starting on L834.

…e-boolean

dsaxton · 2020-09-11T17:36:18Z

pandas/tests/series/indexing/test_mask.py


    rs = s2.where(~cond[:3], -s2)
    rs2 = s2.mask(cond[:3], -s2)
-    tm.assert_series_equal(rs, rs2)
+    # tm.assert_series_equal(rs, rs2)


Temporarily commented this and the test above which are failing. I think the expected output of these are incorrect, since when we have missing values in the mask (e.g. through misalignment), we can't expect the mask and where methods to be perfect inverses of each other.

That is, if we take the complement of the mask then we aren't doing anything to the NA values, so if we pass this to either method and both treat this as False, then we'll see different behavior. To me this is expected:

>>> import numpy as np >>> from pandas import Series >>> >>> s = Series(np.random.randn(5)) >>> cond = Series([True, False, False, True, False], index=s.index) >>> s2 = -(s.abs()) >>> mask = cond[:3] >>> >>> print(s2) 0 -1.150047 1 -1.152933 2 -0.442946 3 -0.705565 4 -0.484288 dtype: float64 >>> print(mask) 0 True 1 False 2 False dtype: bool >>> print(s2.where(~mask)) 0 NaN 1 -1.152933 2 -0.442946 3 NaN 4 NaN dtype: float64 >>> print(s2.mask(mask)) 0 NaN 1 -1.152933 2 -0.442946 3 -0.705565 4 -0.484288 dtype: float64

What do you think @jreback ?

~~If I switch the expected to use invert=True the tests pass so doing that instead~~

…e-boolean

jbrockmendel · 2020-09-16T02:54:39Z

seems like this is a workaround because BooleanArray doesn't have a __inv__. would defining that solve this issue?

jreback · 2020-09-16T08:12:22Z

this is a good point @jbrockmendel

dsaxton · 2020-09-16T14:23:58Z

seems like this is a workaround because BooleanArray doesn't have a __inv__. would defining that solve this issue?

Do you mean the issue with - vs ~? I might be misunderstanding but inverting a BooleanArray seems to work okay:

[ins] In [3]: arr
Out[3]: 
<BooleanArray>
[True, False, <NA>]
Length: 3, dtype: boolean

[ins] In [4]: ~arr
Out[4]: 
<BooleanArray>
[False, True, <NA>]
Length: 3, dtype: boolean

This PR mostly became complicated because of the existing logic surrounding inplace inside _where, and when the inversion should happen in the presence of missing values.

…e-boolean

jreback · 2020-09-19T02:11:46Z

pandas/core/frame.py

@@ -3104,7 +3104,7 @@ def _setitem_frame(self, key, value):

        self._check_inplace_setting(value)
        self._check_setitem_copy()
-        self._where(-key, value, inplace=True)
+        self._where(key, value, inplace=True, invert=True)


so I would remove the invert keyword and just fix panda/core/series as well (and have _where always invert)

Hmm, I think it could be confusing if the default behavior of _where is to do essentially the opposite of what it's done previously.

This keyword is only to handle the special case of inverting after filling when dealing with missing values / misalignment, and while it's not pretty it's at least not user-facing. I'm not really aware of an elegant way to do this given that realignment is already happening inside _where.

_where is private so this doesn't matter, we want as consistent AND minimal api internally as possible.

I can try to get this to work, but changing _where globally in this way breaks a large number of tests

jreback · 2020-09-19T02:11:53Z

pandas/core/generic.py

@@ -8857,13 +8857,18 @@ def _where(
        level=None,
        errors="raise",
        try_cast=False,
+        invert=False,


see my comments above

pandas/core/generic.py

simonjayhawkins · 2020-09-19T13:08:26Z

this is an intricate change and not appropriate for 1.1.2

As this PR evolves, can we review the milestone when this PR is ready but before this is merged.

…e-boolean

simonjayhawkins · 2020-09-29T19:16:40Z

doc/source/whatsnew/v1.2.0.rst

@@ -305,6 +305,7 @@ Indexing
 Missing
 ^^^^^^^

+- Bug in :meth:`DataFrame.mask` and :meth:`DataFrame.where` raising an ``AssertionError`` when using a nullable boolean mask (:issue:`35429`)


This is taking the approach of treating NA as False within NDFrame.mask and NDFrame.where when dealing with a nullable boolean mask.

hmm, thinking some more...

from https://pandas.pydata.org/pandas-docs/dev/reference/api/pandas.DataFrame.mask.html

Where cond is False, keep the original value. Where True, replace with corresponding value from other.

and

>>> bool(pd.NA) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "pandas\_libs\missing.pyx", line 360, in pandas._libs.missing.NAType.__bool__ raise TypeError("boolean value of NA is ambiguous") TypeError: boolean value of NA is ambiguous >>>

maybe this isn't a bug and this should raise, but not AssertionError, see #35429 (comment)

otherwise I think the docs should to be updated to be explicit about the behaviour.

We could be in danger here of introducing a behaviour that we would want to deprecate in the future.

i tend to agree with this

Because mask / where are used in other places that would mean those other cases are left broken as well, e.g., #36395 (referring specifically to the DataFrame indexing example)

The example in #36395 looks like its trying to use np.array([[True, pd.NA]], dtype=object) as a boolean mask, which i think should raise (thought not AssertionError)

(with 2D EAs we can make this a little more nicely behaved and have BooleanArray([[True, pd.NA]])) and get a nicer exception message, but it still isn't clear that should be allowed

I think allowing indexing with nullable booleans is more useful than raising personally. This already works for single 1D arrays (the same way it would when filtering with a nullable boolean column in SQL):

[ins] In [11]: s Out[11]: 0 1 1 2 2 3 dtype: Int64 [ins] In [12]: mask Out[12]: 0 True 1 False 2 <NA> dtype: boolean [ins] In [13]: s[mask] Out[13]: 0 1 dtype: Int64

The argument for allowing it for mask / where as well for me seems to be that these are very much like indexing operations, for which it's useful to treat as above.

simonjayhawkins · 2020-09-29T19:21:42Z

pandas/tests/series/indexing/test_where.py

+        result = ser.copy()
+        result.where(mask, 999, inplace=True)
+    else:
+        result = ser.where(mask, 999, inplace=False)


following on from previous comment.

>>> ser = pd.Series([1, 2, 3], dtype="Int64") >>> mask = pd.Series([True, False, None], dtype="boolean") >>> ser.where(mask, 999) 0 1 1 999 2 999 dtype: Int64

using a nullable mask with a nullable type gives the wrong output?

This output is correct IMO, but the output changes if the operation is inplace:

[ins] In [4]: s.where(mask, 999) Out[4]: 0 1 1 999 2 999 dtype: Int64 [ins] In [5]: s.where(mask, 999, inplace=True) [ins] In [6]: s Out[6]: 0 1 1 999 2 3 dtype: Int64

…e-boolean

jreback · 2020-10-10T23:20:16Z

pandas/core/frame.py

@@ -3104,7 +3104,7 @@ def _setitem_frame(self, key, value):

        self._check_inplace_setting(value)
        self._check_setitem_copy()
-        self._where(-key, value, inplace=True)
+        self._where(key, value, inplace=True, invert=True)


_where is private so this doesn't matter, we want as consistent AND minimal api internally as possible.

dsaxton · 2020-11-06T18:03:18Z

Closing for now since it seems it isn't clear if / how we want to do this

BUG: Don't raise for NDFrame.mask with nullable boolean

4197885

dsaxton added Bug NA - MaskedArrays Related to pd.NA and nullable extension arrays labels Sep 7, 2020

dsaxton added this to the 1.1.3 milestone Sep 7, 2020

dsaxton commented Sep 7, 2020

View reviewed changes

dsaxton added 2 commits September 7, 2020 15:54

Note

0ff4f41

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

d79fcc7

…e-boolean

Dummy commit

6be29fb

jreback requested changes Sep 8, 2020

View reviewed changes

dsaxton removed this from the 1.1.3 milestone Sep 8, 2020

dsaxton added 2 commits September 8, 2020 18:36

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

17bdca0

…e-boolean

Move note

aa3c357

simonjayhawkins added this to the 1.1.3 milestone Sep 9, 2020

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

566d0d6

…e-boolean

jreback requested changes Sep 11, 2020

View reviewed changes

jreback modified the milestones: 1.1.3, 1.2 Sep 11, 2020

dsaxton added 5 commits September 11, 2020 08:15

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

d6998dd

…e-boolean

Move note

8d35401

Invert in _where

3169909

Comment out some tests for now

db45e2e

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

a1fcf51

…e-boolean

dsaxton commented Sep 11, 2020

View reviewed changes

dsaxton added 3 commits September 11, 2020 13:09

xor

bd584da

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

f2e8b7d

…e-boolean

Doc

ea457ea

dsaxton added 3 commits September 13, 2020 17:08

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

b40243e

…e-boolean

Boolean magic

363560c

Only comment

2020fd6

dsaxton mentioned this pull request Sep 16, 2020

BUG: indexing with DataFrame with nullable boolean dtype #36395

Closed

dsaxton added 3 commits September 16, 2020 10:31

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

1cf5a0c

…e-boolean

Add frame indexing test

d6fb23a

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

e292606

…e-boolean

jreback requested changes Sep 19, 2020

View reviewed changes

dsaxton added 4 commits September 19, 2020 10:31

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

2f024fe

…e-boolean

Remove arg

0b90786

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

054a8cd

…e-boolean

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

cd601f6

…e-boolean

dsaxton mentioned this pull request Sep 21, 2020

BUG: pandas.DataFrame.where doesn't work when cond is of Pandas Dtype #36521

Closed

3 tasks

dsaxton added 6 commits September 22, 2020 19:44

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

424b6bd

…e-boolean

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

8e8cb9d

…e-boolean

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

2151cc0

…e-boolean

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

59cf6a1

…e-boolean

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

dfb7b59

…e-boolean

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

a313268

…e-boolean

simonjayhawkins reviewed Sep 29, 2020

View reviewed changes

dsaxton added 2 commits October 7, 2020 11:05

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

ee2849c

…e-boolean

Merge remote-tracking branch 'upstream/master' into mask-with-nullabl…

5a4bacf

…e-boolean

jreback requested changes Oct 10, 2020

View reviewed changes

dsaxton closed this Nov 6, 2020

dsaxton deleted the mask-with-nullable-boolean branch March 30, 2021 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Don't raise for NDFrame.mask with nullable boolean #36201

BUG: Don't raise for NDFrame.mask with nullable boolean #36201

dsaxton commented Sep 7, 2020 •

edited

Loading

dsaxton Sep 7, 2020

simonjayhawkins commented Sep 7, 2020

dsaxton commented Sep 7, 2020

dsaxton commented Sep 7, 2020

jreback left a comment

simonjayhawkins commented Sep 9, 2020

jreback left a comment

jreback Sep 11, 2020

dsaxton Sep 11, 2020

dsaxton Sep 13, 2020 •

edited

Loading

jbrockmendel commented Sep 16, 2020

jreback commented Sep 16, 2020

dsaxton commented Sep 16, 2020

jreback Sep 19, 2020

dsaxton Sep 19, 2020

jreback Oct 10, 2020

dsaxton Oct 11, 2020

jreback Sep 19, 2020

simonjayhawkins commented Sep 19, 2020

simonjayhawkins Sep 29, 2020

jbrockmendel Sep 29, 2020

dsaxton Sep 29, 2020 •

edited

Loading

jbrockmendel Oct 7, 2020

dsaxton Oct 7, 2020

simonjayhawkins Sep 29, 2020

dsaxton Oct 7, 2020

jreback Oct 10, 2020

dsaxton commented Nov 6, 2020

BUG: Don't raise for NDFrame.mask with nullable boolean #36201

BUG: Don't raise for NDFrame.mask with nullable boolean #36201

Conversation

dsaxton commented Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

simonjayhawkins commented Sep 7, 2020

dsaxton commented Sep 7, 2020

dsaxton commented Sep 7, 2020

jreback left a comment

Choose a reason for hiding this comment

simonjayhawkins commented Sep 9, 2020

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsaxton Sep 13, 2020 • edited Loading

Choose a reason for hiding this comment

jbrockmendel commented Sep 16, 2020

jreback commented Sep 16, 2020

dsaxton commented Sep 16, 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

simonjayhawkins commented Sep 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsaxton Sep 29, 2020 • 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

dsaxton commented Nov 6, 2020

dsaxton commented Sep 7, 2020 •

edited

Loading

dsaxton Sep 13, 2020 •

edited

Loading

dsaxton Sep 29, 2020 •

edited

Loading