Parametrized NA sentinel for factorize
State: closed

Adds a new keyword `na_value` to control the NA sentinel inside the factorize
routine.

```python
In [3]: arr = np.array([0, 1, 0, 2], dtype='u8')

In [4]: pd.factorize(arr)
Out[4]: (array([0, 1, 0, 2]), array([0, 1, 2], dtype=uint64))

In [5]: pd.factorize(arr, na_value=0)
Out[5]: (array([-1,  0, -1,  1]), array([1, 2], dtype=uint64))
```

The basic idea is that our hashtables now have two "modes" for detecting NA values

1. The previous way, based on the templated na_condition
2. Equality to a user-specified NA value.

Note: specifying NA value does not "turn off" the old way of checking NAs.

```python
In [2]: pd.factorize(np.array([0.0, np.nan, 1.0]), na_value=0.0)
Out[2]: (array([-1, -1,  0]), array([1.]))
```

I'm sure the implementation can be cleaned up, but I wanted to put this up for feedback. Hopefully someone can tell me how to avoid the `use_default_na` nonsense :)

cc @WillAyd

closes https://github.com/pandas-dev/pandas/issues/20328

Labels:
name: Missing-data
description: np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate
name: Algos
description: Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20473?src=pr&el=h1) Report
> Merging [#20473](https://codecov.io/gh/pandas-dev/pandas/pull/20473?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/6c0c27730fb5b20978824fb380449cd3acef55f7?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/20473/graphs/tree.svg?width=650&height=150&src=pr&token=eZ4WkYLtcO)](https://codecov.io/gh/pandas-dev/pandas/pull/20473?src=pr&el=tree)

```diff
@@           Coverage Diff           @@
##           master   #20473   +/-   ##
=======================================
  Coverage   91.82%   91.82%           
=======================================
  Files         152      152           
  Lines       49235    49235           
=======================================
  Hits        45212    45212           
  Misses       4023     4023
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.21% <100%> (ø)` | :arrow_up: |
| #single | `41.89% <87.5%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/20473?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/dtypes/missing.py](https://codecov.io/gh/pandas-dev/pandas/pull/20473/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL21pc3NpbmcucHk=) | `91.07% <ø> (ø)` | :arrow_up: |
| [pandas/core/arrays/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/20473/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2NhdGVnb3JpY2FsLnB5) | `96.19% <100%> (-0.02%)` | :arrow_down: |
| [pandas/core/algorithms.py](https://codecov.io/gh/pandas-dev/pandas/pull/20473/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYWxnb3JpdGhtcy5weQ==) | `94.31% <100%> (+0.01%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20473?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20473?src=pr&el=footer). Last update [6c0c277...465d458](https://codecov.io/gh/pandas-dev/pandas/pull/20473?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Thanks to @WillAyd we've cleaned up some of the hackiness.

Now I'd like to see if we can remove some of the strangeness around the two ways of having nulls, `{{null_condition}}` or a user-specified NA value.

And more tests.
@TomAugspurger did the below not work to consolidate the "two null methods"?

```python
if (check_null and (val != val or val == na_value))
```

If so we could remove `null_condition` from the templating altogether (would make it more consistent with groupby). I don't think you need to have `use_na_value` as part of the conditional because that should be setting the appropriate na_value earlier in the method (maybe?)
👍 I had to modify `check_null` to be True whenever `na_value` is specified.

I want to add more tests around the behavior of specifying an `na_value` and existing NA values (`NaN`, `NaT`, None, etc.). Do we agree with the following behavior?

```python
In [17]: pd.factorize([0.0, np.nan, 1.0], na_value=0.0)
Out[17]: (array([-1, -1,  0]), array([1.]))
``` 

i.e. NaN will always be NA. This is currently broken for NaT I think.


```python
In [19]: pd.factorize(pd.DatetimeIndex([pd.Timestamp(0), pd.Timestamp(1), None]), na_value=0)
Out[19]:
(array([-1,  0,  1]),
 DatetimeIndex(['1970-01-01 00:00:00.000000001', 'NaT'], dtype='datetime64[ns]', freq=None))
```

That should be `[-1, 0, -1]` for the labels I think...
I actually think the first example should raise a `ValueError`, if only because I don't see a reason why a person would intentionally provide a `na_value` in an array that contains NA. The reasoning for that is unclear and I think it would be better to signal to the user that something may be awry (they probably wanted to fill the missing value first?)

For the second example, I also would prefer a ValueError gets thrown when the type of na_value does not match the type of the values being factorized (not applicable for object dtypes). `pd.Timestamp(0) == 0` is False so I wouldn't expect `factorize` to roll its own equality comp.

Ignoring that, depending on how you feel about my stance on example one the second example should either raise a ValueError or go `[-1, 0, -1]` as you mentioned. Just my $.02
Thinking more on this, I think we should not make the `na_value` parameter public to end users. I think it should only be available in the private `_factorize_array` method. Two reasons

1. Some awkwardness around calling `pd.factorize(Categorical, na_value=foo)`. That "works", but the `na_value` is ignored since the chain is `pd.factorize` -> `Categorical.factorize` -> `pd.factorize`, and `Categorical.factorize` doesn't take `na_value` so it's dropped.
2. The only concrete use-cases we have is extension arrays (including internal ones). In this case, we won't be mixing multiple missing markers. We can either tell EA authors that `_factorize_array` is public to them, or just ask them "what's your NA value" and pass it to `_factorize_array` in the default implementation for them.
Updated to move the parameter just to `_factorize_array`, and not the public `factorize`. I've tested a variant of https://github.com/pandas-dev/pandas/pull/20361 built on this. It works well for JSONArray, IPArray (object hashtable) and MACArray (uint64 hashtable). And of course Categorical, which I've changed here to use `na_value`.
Removed `check_nulls` entirely.

I removed `DatetimeIndexOpsMixin.factorize` and put the logic in `pandas.factorize`. It has to go there so that we correctly handle `ndarray[datetime64ns]`.
I'm running the tests locally with `na_value_for_dtype(Period)` returning NaT. If that passes I'll change `factorize` to use it.
@jreback are you OK with this now, including the changes to `na_value_for_dtype(Period)`? Would like to get this merged so I can finish of factorize, and make a PR for groupby.
Ran the ASV, and nothing stood out.
thanks @TomAugspurger