API: membership checks on ExtensionArray containing NA values
State: closed

Membership checks on ExtensionArrays containing `NA` values raises ValueError in some circumstances (but not in other):

```python
>>> arr1 = pd.array(["a", pd.NA])
>>> arr2 = pd.array([pd.NA, "a"])
>>> "a" in arr1
True  # ok
>>> "a" in arr2
TypeError: boolean value of NA is ambiguous  # not ok
>>> pd.NA in arr1
TypeError: boolean value of NA is ambiguous  # not ok
>>> pd.NA in arr2
True  # ok
```

So overall quite random failures. This PR fixes this problem by adding a custom `__contains__` method on `ExtensionArray`.

I assume that we want `pd.NA in arr1` to keep returning True. Note however that `np.nan in np.array([np.nan])` return False, so pandas' behaviour is different.

Labels:
name: Indexing
description: Related to indexing on series/frames, not to indexes themselves
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

Comments:
ok so this looks fine. merge and then open issues / followup for recently discovered cases?
Now, I don't really know myself at the moment what I would expect for behaviour here. Listing all possible options (eg for `pd.NA in pd.array([1, 2, pd.NA])`:

* Return True (because pd.NA is present in the array)
* Return False (because pd.NA is not equal to itself, so cannot be "found")
* Raise an error (because it is ambiguous (since there is an argument for both True/False), or because there is simply no "correct" answer, in the same logic as we also raise an error for `bool(pd.NA)`)
  * Counterargument: in dictionaries pd.NA can be used as key, so you could argue that containement is similar as dictionary key checking
* Return pd.NA (similar reasons as above, + since the pd.NA values are considered as "unknown", it is *also* "unknown" if the unknown value is contained in the array)

Quickly looking at some other languages to see what they do:

R returns True:

```R
> NA %in% c(1, 2, 3, NA)
[1] TRUE
```

Julia is "strict" about the unknown part of `missing`, and if a missing value is present in the array, even any other value not present in the array is not known to *not* be in the array (so not even returns False):

```julia
julia> arr = [1 2 3 missing]
1×4 Array{Union{Missing, Int64},2}:
 1  2  3  missing

julia> 1 in arr
true

julia> 5 in arr
missing

julia> missing in arr
missing

julia> arr = [1 2 3]
1×3 Array{Int64,2}:
 1  2  3

julia> 5 in arr
false

julia> missing in arr
missing
```

Yes, @jorisvandenbossche, my doubts on nan-likes as well: There doesn't seem to be any inherantly correct choice, as it look to me. 

We have however already in pandas that:

```python
>>> arr2 = pd.array([pd.NA, "a"])
>>> pd.NA in arr2  # see OP
True
>>> dta = pd.date_range("2016-01-01", periods=2)._data
>>> dta[0] = pd.NaT
>>> pd.NaT in dta
True
```

So from at backward compat stand point there is some argument for returning True in Pandas, if looking for a nan-like in the ExtensionArray.
Specifically that `pd.NA in pd.array([pd.NA, "a"])` is returning True, I wouldn't use as a "prior" art argument, as I think this is mostly accidental behaviour and we never really considered yet the desired behaviour for `__contains__` for nullable EAs (and those types are not yet considered stable, so we can change behaviour if we decide this is needed).
 
The fact that it returns True for pd.NaT certainly has a much longer history, but I don't know to what extent this was intentional (@jbrockmendel ?). For example the numpy counterpart actually returns False:

```
In [20]: arr = np.array(["NaT"], dtype="datetime64[ns]")

In [21]: arr[0]
Out[21]: numpy.datetime64('NaT')

In [22]: arr[0] in arr
Out[22]: False
```

---

Something else, the current `isna(item)` check is also too broad, as (I think from looking at the code), it will make that things like `None in pd.array([pd.NA, "a"])` or `np.nan in pd.array([pd.NA, "a"])` will also return True, which I think we certainly don't want?


> The fact that it returns True for pd.NaT certainly has a much longer history, but I don't know to what extent this was intentional

I'm pretty sure that is unintentional.  I've been fixing some `isna` checks to use `is_valid_nat_for_dtype`, but haven't gotten all of them.

> Something else, the current `isna(item)` check is also too broad, as (I think from looking at the code), it will make that things like `None in pd.array([pd.NA, "a"])` or `np.nan in pd.array([pd.NA, "a"])` will also return `True`, which I think we certainly don't want?

I'd advocate updating `is_valid_nat_for_dtype` to reflect those exclusions.  (And renaming `valid_nat` -> `valid_na`)

> ```
> In [22]: arr[0] in arr
> Out[22]: False
> ```

That is surprising.  My intuition is that `arr[0] in arr` should be `True` for _any_ `arr`.  I'm open to being convinced otherwise.
> That is surprising.  My intuition is that `arr[0] in arr` should be `True` for _any_ `arr`.  I'm open to being convinced otherwise.

For NaN/NaT, those values are not equal to itself, and I think numpy uses equality to implement `__contains__`
> For NaN/NaT, those values are not equal to itself, and I think numpy uses equality to implement __contains__

My intuition is based more around our Index implementations that use `get_loc`.

What would the idiomatic way of checking `any(x is pd.NA for x in arr)` be?
> What would the idiomatic way of checking `any(x is pd.NA for x in arr)` be?

`arr.isna().any()` IMO.

I think there will be be a surprise if users check for nan-likes using `__contains__` for some users, because there are two reasoably choices:

1. `pd.NA in arr` should be interpreted as `(pd.NA == arr).any()`, resulting in `False` even if a nan-like is in `arr`
2. `pd.NA in arr` should be interpreted as `arr.isna().any()`, resulting in `True` if a nan-like is in `arr`, but will also return True for e.g. `pd.NaT in arr`, which may be unintended (or has to be decided upon.

A solution to avoid ambiguity is to raise a TypeError, if a nan-like is supplied to `__contains__`: 

```python
    def __contains__(self, item) -> bool:
        if isna(item):
            raise TypeError("NA value not allowed. Use .isna method to check for NA values.")
        else:
            return (item == self).any()
```

This would at least ensure that `"a" in arr` doesn't raise unexpectedly.
>> What would the idiomatic way of checking any(x is pd.NA for x in arr) be?

> arr.isna().any() IMO.

My complaint about that is it doesn't distinguish between different NAs, most relevant for object dtype.

> This would at least ensure that "a" in arr doesn't raise unexpectedly.

Short-term, can we fix this without taking a stance on the NA topic?

My intuition is that `obj in pd.array(arr)` should match `obj in pd.Index(arr)` and `obj in pd.Series(arr)`.  Is this controversial?
> > > What would the idiomatic way of checking any(x is pd.NA for x in arr) be?
> 
> > arr.isna().any() IMO.
> 
> My complaint about that is it doesn't distinguish between different NAs, most relevant for object dtype.
> 
> > This would at least ensure that "a" in arr doesn't raise unexpectedly.
> 
> Short-term, can we fix this without taking a stance on the NA topic?
> 
> My intuition is that `obj in pd.array(arr)` should match `obj in pd.Index(arr)` and `obj in pd.Series(arr)`. Is this controversial?

agree with @jbrockmendel 

I think have NaN != itself is different then a membership test. we do not want special cases.
> Short-term, can we fix this without taking a stance on the NA topic?

How is fixing this not taking a stance? ;) 
Because to fix it we need to choose a certain behaviour regardless, which then defacto becomes the `__contains__` behaviour for pd.NA ..

The `obj in pd.Index(arr)` example from @jbrockmendel is a good comparison, because it basically does a engine/hashtable lookup operation under the hood and so I suppose will be consistent with indexing (`series.loc[pd.NA]` if the index has missing values). 

So that basically gives two possible interpretations / implementations of `__contains__`:

- equality-based: (`(arr == obj).any()`)
- lookup-based (eg `pd.NA in dict.fromkeys(arr)`, or our indexing engine lookup)

While numpy uses the first one, it seems to make sense to use the second option for membership test like `__contains__`.  

But, I think there is also something to say for using the "unknown value" interpretation of `pd.NA`. And then we don't know if some "unknown value" is present in the array, and we shouldn't return True or False.


@topper-123 since you are adding a method on the base extension array class, I think we should also add a test for this in the generic base extension tests. There are fixtures for data with/without missing values and for the na_value, so it should be possible to test this in general.
> 
> 
> @topper-123 since you are adding a method on the base extension array class, I think we should also add a test for this in the generic base extension tests. There are fixtures for data with/without missing values and for the na_value, so it should be possible to test this in general.

I've looked, but can't find the base extension tests. Could you say where they're located?
In `pandas/tests/extension/base/`, and then in one of those files. Not sure which file is best fitting, maybe `interface.py`, or `methods.py`
>> Short-term, can we fix this without taking a stance on the NA topic?

> How is fixing this not taking a stance? ;)

I was specifically referring to "This would at least ensure that `"a" in arr` doesn't raise unexpectedly.", which I think we can ensure without taking a stance on the behavior of `pd.NA in arr`.
> I was specifically referring to "This would at least ensure that `"a" in arr` doesn't raise unexpectedly."

Ah, yes, that's something we certainly agree on I think that this at least shouldn't raise! 
(but to fix that we still need to choose a behaviour for `pd.NA in ..` as well)

Now, I am fine with following our behaviour for Index containment / indexing, which is to treat NaN and pd.NA etc equal to itself (in contrast to `==` equality).
I've updated the PR.

For comparisons with nan_likes, it nows returns True if the na value is `arr.dtype.na_value` else False.
The Travis failure looks unrelated. I'll run the PR again after comments.
The travis failure looks unrelated and I can't reproduce it on my computer. So IMo this PR passes currently, but I could make another run, if there's agreement on the content.
Yes, don't worry about the travis failure on this PR
lgtm. thanks @topper-123