BUG: fix usage of na_sentinel with sort=True in factorize()
State: closed

Closes https://github.com/pandas-dev/pandas/issues/25409


Labels:
name: Regression
description: Functionality that used to work in a prior pandas version
name: Algos
description: Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25592?src=pr&el=h1) Report
> Merging [#25592](https://codecov.io/gh/pandas-dev/pandas/pull/25592?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/b72e7ed320de4d5b9aa8e0b7f2166cee2c7e6fdf?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #25592      +/-   ##
==========================================
+ Coverage   91.26%   91.26%   +<.01%     
==========================================
  Files         173      173              
  Lines       52966    52968       +2     
==========================================
+ Hits        48338    48340       +2     
  Misses       4628     4628
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.83% <100%> (ø)` | :arrow_up: |
| #single | `41.71% <75%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25592?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/algorithms.py](https://codecov.io/gh/pandas-dev/pandas/pull/25592/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYWxnb3JpdGhtcy5weQ==) | `94.79% <100%> (+0.01%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25592?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/25592?src=pr&el=footer). Last update [b72e7ed...7356997](https://codecov.io/gh/pandas-dev/pandas/pull/25592?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25592?src=pr&el=h1) Report
> Merging [#25592](https://codecov.io/gh/pandas-dev/pandas/pull/25592?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/dc7b4662b256b5519dce86ec3e3e12754da34b18?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #25592      +/-   ##
==========================================
- Coverage   91.29%   91.29%   -0.01%     
==========================================
  Files         173      173              
  Lines       52961    52963       +2     
==========================================
+ Hits        48350    48351       +1     
- Misses       4611     4612       +1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.87% <100%> (ø)` | :arrow_up: |
| #single | `41.73% <75%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25592?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/algorithms.py](https://codecov.io/gh/pandas-dev/pandas/pull/25592/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYWxnb3JpdGhtcy5weQ==) | `94.79% <100%> (+0.01%)` | :arrow_up: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/25592/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `88.98% <0%> (-0.1%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25592?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/25592?src=pr&el=footer). Last update [dc7b466...a9c880e](https://codecov.io/gh/pandas-dev/pandas/pull/25592?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

not sure why this is that important for 0.24.2. fixing this properly would block the release which doesn't make much sense.
I think it's essentially ready though, right?
this is just adding technical debt and am -1 on merging as is. Also the idea of merging a patch then immediately 'fixing' it in master is better, but still, why the urgency. This is a very small edge case, yet adding a horrible hack.
> Also the idea of merging a patch then immediately 'fixing' it in master is better, but still, why the urgency.

If you say it is better, do you also mean you are OK with that?

For sure it is not the most important bug fix, but it is a very clear error (the output is simply total nonsense), for which we have a fix that we can just merge, so why not?
Can you explain the tech-debt comment? I still don't quite understand that.

Would you prefer the `try / except` be moved inside its own function? I prefer not to make functions unless they're used in more than one place, since it makes the code harder to follow.
> If you say it is better, do you also mean you are OK with that?

no


> Can you explain the tech-debt comment? I still don't quite understand that.>

> Would you prefer the try / except be moved inside its own function? I prefer not to make functions unless they're used in more than one place, since it makes the code harder to follow.

well this is adding multiple try/excepts with the same code. This is horrible.
again explain the urgency. this is NOT in any way important at all.
As I said above, it is indeed not urgent. But it is a ready fix for a known regression. So again, why not?

You say you want a proper solution. Can you then explain what you mean with this? What do you propose?

> well this is adding multiple try/excepts with the same code. This is horrible.

It is not adding multiple try/excepts. There was one, and there is still one. I only added one if/else case to check for a keyword argument depending on which the try/except should never be tried at all.
How should we proceed then? 

> On Mar 12, 2019, at 08:12, Jeff Reback <notifications@github.com> wrote:
> 
> again explain the urgency. this is NOT in any way important at all.
> 
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub, or mute the thread.

i see no reason to merge this

this is making already complicated code
way more complicated 
Our current code is simply wrong. If you don't want the proposed solution here, can you propose an alternative solution?
Joris, maybe try moving the if condition into safe_sort itself? It’ll be the same code, but maybe a new diff would be refreshing. 
Actually, doing the `if na_sentiel == -1` check in `safe_sort` isn't quite straightforward, as that accepts list-like and we need array-like.

I'm still not sure where the "way repetitive" code is. The only repetition I see is

```
                uniques, labels = safe_sort(uniques, labels,
                                             na_sentinel=na_sentinel,
                                             assume_unique=True)
```

Am I missing something?
An additional problem is that `safe_sort` handles more cases, eg it also checks for out of bound labels (which we know we don't have here). So if using `take_1d` inside `safe_sort`, we would need extra code there to also handle out of bound labels when using `take_1d`
I looked a bit further in it. It is possible to move the logic into `safe_sort`, with:

- adding a `check_outofbounds` keyword to disable extra checks (otherwise the performance benefit of `take_1d` is lost)
- fixing `safe_sort` to work for EAs

how it looks like: https://github.com/jorisvandenbossche/pandas/commit/ba944eb09a784605db9a3d51aa154f1f29aa7a5f

Would that be a more preferred solution?
A bit less clear, but perhaps better since all EAs can benefit from it?

On Tue, Mar 12, 2019 at 1:05 PM Joris Van den Bossche <
notifications@github.com> wrote:

> I looked a bit further in it. It is possible to move the logic into
> safe_sort, with:
>
>    - adding a check_outofbounds keyword to disable extra checks
>    (otherwise the performance benefit of take_1d is lost)
>    - fixing safe_sort to work for EAs
>
> how it looks like: jorisvandenbossche@ba944eb
> <https://github.com/jorisvandenbossche/pandas/commit/ba944eb09a784605db9a3d51aa154f1f29aa7a5f>
>
> Would that be a better solution?
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/25592#issuecomment-472116855>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIr6Zo_gFfZrdEZ63oE8zZwC7Dfh4ks5vV-xxgaJpZM4bjZIu>
> .
>

The discussion is mainly about a code style issue (a way to prevent calling ``safe_sort`` twice as Jeff proposes, vs calling the function twice as in the current diff), and not about the actual proposal (falling back to `safe_sort` for the impacted case). And code style is subjective, given that we disagree over which of two is better. But since they are both basically equivalent, I would say this is a minor issue.

With my release manager hat on (but I know, I am not really independent here), I am going to decide not to block this PR over such a minor issue. So I will merge this PR. But, to address the discussion here, I will also open a new PR with my other proposal mentioned above (moving the logic to `safe_sort` and fixing that to handle EAs). But since this touches more parts, I prefer to keep this for 0.25.0.
PR is here: https://github.com/pandas-dev/pandas/pull/25696
I am -1 on doing this again.  merging bad code is not great. here. There is no urgency on this at all. We hold up PRs all the time on code style.