ENH: Implement Kleene logic for BooleanArray
State: closed

xref #29556 

I have a few TODOs, and a few tests that I need to unxfail. Putting this up now so that @jorisvandenbossche can take a look.

Labels:
name: Missing-data
description: np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

Comments:
Sorry @jorisvandenbossche, I pushed a somewhat major refactor after you reviewed :/ Should be done now.

High-level overview:

* Split into dedicated methods `kleene_or`, `kleene_and`, and `kleene_xor`, rather than doing everything in `_create_logical_method`.
* Removed the `_compare_other`, `test_scalar`, and `test_array` tests. Since there were parameterized over all the ops, we would have needed to essentially re-implement the kleene logic in `_compare_other` to get the masking right.
* Added tests for each op against both arrays and scalars (True, False, np.nan)

My main concern right now is that I may be assuming that masked values are false in a few places.

---

One important comment was buried

> But that gives the question: what do we want a | np.nan to do?

Right now, I've adopted `pd.NA` semantics. I think we do that or raise, I don't have a preference. Easy to support both.

 

> My main concern right now is that I may be assuming that masked values are false in a few places.

How would you be assuming that? Is there a place that you "uncover" masked values?
> Right now, I've adopted pd.NA semantics. I think we do that or raise, I don't have a preference. Easy to support both.

I would maybe rather raise an error then. As otherwise you have a `np.nan` that behaves differently depending on the context.
> How would you be assuming that? Is there a place that you "uncover" masked values?

Still thinking through it, but we do things rougly like

```python
result = left & right
...
mask[result] = False
```

i.e. we update the mask based on the result. I'll add some tests where I manually modify the `_data` of masked values to be True.
> i.e. we update the mask based on the result. 

Ah, I see. Yes, that shouldn't be done. Or it should combine with mask from the original values first, I think?
Another batch of changes

* raise for NaN: https://github.com/pandas-dev/pandas/pull/29842/commits/7f78a64faee7d9be2c88990f3f930fa89fae2d11
* More tests (empty arrays, inplace mutation) https://github.com/pandas-dev/pandas/pull/29842/commits/747e046ed54866498260f5922fdbc44eb0e4a85f, https://github.com/pandas-dev/pandas/pull/29842/commits/36b171b2ff1f01563367c13313572d257d7d67fc
* Ensure we aren't relying on the assumption that masked values are False: https://github.com/pandas-dev/pandas/pull/29842/commits/d0a8cca3d8499e62c5488612a527756e7ad9057e

The tests in https://github.com/pandas-dev/pandas/pull/29842/commits/d0a8cca3d8499e62c5488612a527756e7ad9057e are a bit tricky... Hopefully they're comprehensive and make sense.
I think a similar optimization as I did for "and" should also be possible for "or"
> I think a similar optimization as I did for "and" should also be possible for "or"

Done in https://github.com/pandas-dev/pandas/pull/29842/commits/7b9002cf52e93ba49bb425da6abf09b3fb0cf529.
xref #2434?
Also xref #6528 #22724. Big +1
@TomAugspurger it might be best to first finish NA in BooleanArray in https://github.com/pandas-dev/pandas/pull/29961, and then update this PR to also handle the scalar cases?
@jorisvandenbossche yes, I think that order makes sense.
This may have come up, but FYI, _libs.ops has a few functions that implement kleen-adjacent logic, might be worth using/adapting
Are you referring to `vec_binop` and `scalar_binop`? If so, I don't think there's anything there that can be reused. From what I can tell, those just pass through the one value when the other is `NA`.

```python
        if x is None or is_nan(x):
            result[i] = x
        else:
            result[i] = op(x, val)
```

For kleene-logic, whether the NA is propagated depends on the operation being performed, and potentially the non-NA value itself.
I've moved the `kleene*` functions to nanops. I don't have a strong preference for where they live.
> I've moved the kleene* functions to nanops. I don't have a strong preference for where they live.

Currently the nanops only hold reductions (there are actually nancmp, but they are not used AFAIK), so not fully sure it necessarily fits there better (certainly if it doesn't use any of the machinery of that file). And it also already a quite long file. 
(maybe we should start a "maskops" file ..)
But, although my comment above, I also don't care much where it is put ;)
nanops is nearly all unary ops, where core.ops is mostly binary ops.  So i think these probably belong in core.ops
can you add tests checking that the arrays/masks arent getting modified inplace (which is a problem in IntegerArray)
> can you add tests checking that the arrays/masks arent getting modified inplace (which is a problem in IntegerArray)

@jbrockmendel there are several cases of `# ensure we haven't mutated anything inplace` in the tests already, can you check that those do what you want?


@jorisvandenbossche what did you do about NumPy scalars not returning NotImplemented for certain operations? We're getting the wrong answer for NumPy scalars, since our implementation isn't called.

```python
In [2]: a = pd.array([True, False, None])

In [3]: a | np.bool_(True)
Out[3]:
<BooleanArray>
[True, True, True]
Length: 3, dtype: boolean

In [4]: np.bool_(True) | a
Out[4]:
<BooleanArray>
[True, True, NA]
Length: 3, dtype: boolean
```
Hmm, for the NA scalar itself I skipped the tests, as I thought there was nothing to do about this:

https://github.com/pandas-dev/pandas/blob/ee6e6b38528f88cbdc850a2c3b671825ed73581c/pandas/tests/scalar/test_na_scalar.py#L49-L69

But for the array-level ops this seems even more annoying .. (more likely to run into, as we do return numpy scalars from indexing/ops). 
But I thought that `__array_priority__` should ensure that ops are directed to the BooleanArray? (numpy scalars should have a very low array priority)
I am actually linking to the comparison ops tests, for logical ops I apparently didn't add tests for numpy scalars (something to improve!)
Ahh, we're going through `__array_ufunc__`, which is not using Kleene logic. Should have been obvious that we got control somehow, as that returned a BooleanArray.

Will fix that.
> Should have been obvious that we got control somehow, as that returned a BooleanArray.

Aha, yes missed that as well :)

We probably need to fix this for the other ops as well (can be in another PR)

OK, NumPy bools should be handled now. I did that by handling `or`, `xor`, and `and` in `maybe_dispatch_ufunc_to_dunder_op`. Hopefully that doesn't break anything else.
All green. Should be good to go.
@TomAugspurger happy to merge if @jorisvandenbossche ok
> what did you do about NumPy scalars not returning NotImplemented for certain operations? We're getting the wrong answer for NumPy scalars, since our implementation isn't called.

Not clear that it helps _here_, but it might be relevant that pd.NA has no `__array_priority__` attr
> it might be relevant that pd.NA has no __array_priority__ attr

It indeed doesn't help here (since the object is BooleanArray, not pd.NA, and BooleaArray already has an array_priority set). 
But, I tried it yesterday for pd.NA, as it would be nice to solve those inconsistencies. But I don't see any difference ..

So @jbrockmendel is the following correct? My understanding is that `__array_priority__` ensures that if an operation is done between two objects that both have an `__array_priority__` set, that the object with the highest priority can perform the operation first, even if it is the right hand side object? 

That can ensure that both these operations return a Timestamp:

```
In [15]: pd.Timestamp("2012-01-01") + np.timedelta64(1, 'h')
Out[15]: Timestamp('2012-01-01 01:00:00')

In [16]: np.timedelta64(1, 'h') + pd.Timestamp("2012-01-01") 
Out[16]: Timestamp('2012-01-01 01:00:00')
```

And, indeed, if I comment out `__array_priority__` on `Timestamp`, we get:

```
In [1]: pd.Timestamp("2012-01-01") + np.timedelta64(1, 'm')
Out[1]: Timestamp('2012-01-01 00:01:00')

In [2]: np.timedelta64(1, 'h') + pd.Timestamp("2012-01-01")
---------------------------------------------------------------------------
UFuncTypeError                            Traceback (most recent call last)
<ipython-input-2-0de73ced9bd3> in <module>
----> 1 np.timedelta64(1, 'h') + pd.Timestamp("2012-01-01")

UFuncTypeError: ufunc 'add' cannot use operands with types dtype('<m8[h]') and dtype('O')
```

But if I add `__array_priority__ = 100` to the NAType class, I still see this inconsistent behaviour:

```
In [1]: pd.NA == np.bool_(True) 
Out[1]: NA

In [2]: np.bool_(True) == pd.NA 
Out[2]: False
```

So it seems that the decision who gets priority or which operation is executed first is more complicated (or array priority is not honoured here, which is maybe something to report to numpy?)