Factorize don't preserve na_sentinel values when sort is specified

[rahulnair23]

Code Sample, a copy-pastable example if possible

print("\nWithout sort:")
labels, uniques = pd.factorize(['b', None, 'a', 'c', 'b'], na_sentinel=100)
print("Labels: {}".format(labels))
print("Uniques: {}".format(uniques))

print("\nWith sort:")
labels, uniques = pd.factorize(['b', None, 'a', 'c', 'b'], na_sentinel=100, sort=True)
print("Labels: {}".format(labels))
print("Uniques: {}".format(uniques))


Without sort:
Labels: [  0 100   1   2   0]
Uniques: ['b', 'a', 'c']

With sort:
Labels: [         1 4556668520          0          2          1]
Uniques: ['a', 'b', 'c']

Problem description

Calls to factorize with a na_sentinel value set and sort=True appears not to respect the na_sentinel values specified.

Expected Output

Without sort:
Labels: [  0 100   1   2   0]
Uniques: ['b', 'a', 'c']

With sort:
Labels: [         1 100          0          2          1]
Uniques: ['a', 'b', 'c']

Output of pd.show_versions()

[paste the output of pd.show_versions() here below this line]
INSTALLED VERSIONS

commit: None
python: 3.6.8.final.0
python-bits: 64
OS: Darwin
OS-release: 18.2.0
machine: x86_64
processor: i386
byteorder: little
LC_ALL: None
LANG: en_IE.UTF-8
LOCALE: en_IE.UTF-8

pandas: 0.24.1
pytest: None
pip: 18.1
setuptools: 40.6.3
Cython: None
numpy: 1.16.1
scipy: 1.2.0
pyarrow: None
xarray: None
IPython: 7.2.0
sphinx: None
patsy: None
dateutil: 2.8.0
pytz: 2018.9
blosc: None
bottleneck: None
tables: None
numexpr: None
feather: None
matplotlib: 3.0.2
openpyxl: None
xlrd: None
xlwt: None
xlsxwriter: None
lxml.etree: None
bs4: None
html5lib: None
sqlalchemy: None
pymysql: None
psycopg2: None
jinja2: None
s3fs: None
fastparquet: None
pandas_gbq: None
pandas_datareader: None
gcsfs: None

[jorisvandenbossche]

@rahulnair23 Thanks for the report!

This is indeed clearly a bug (it seems to be a regression as well, although not in the last release. It also fails in 0.23.4, but works correctly in 0.22.0)

[jorisvandenbossche]

This seems to be due to the use of `take_1d:

In [3]: pd.core.algorithms.take_1d(np.array([1, 0, 2]), np.array([0, 100, 1, 2, 0]), fill_value=100)
Out[3]: 
array([                  1, 2308668953584821362,                   0,
                         2,                   1])

but I think this is actually a wrong usage of take_1d (it was introduced in #19938, cc @TomAugspurger). The fill_value is the value which should be used for -1's in the indexer, not to indicate what the missing value is in the indexer.

So if we want to use this faster path with take_1d, I think we first need to replace the fill_value with -1 (and afterwards put it back), or otherwise extend take_1d to be able to work with custom na_sentinel's

[gfyoung]

(it seems to be a regression as well, although not in the last release. It also fails in 0.23.4, but works correctly in 0.22.0)

Removing 0.24.2 since it was explicitly stated that the breakage did not first occur in 0.24.0.

[jorisvandenbossche]

@gfyoung there is a reason I added it explicitly to the milestone: it is still a recent regression, it is related to the refactoring we did, and I already basically mentioned how to solve it. Therefore, I think it could be good to include it. So added back the milestone, to at least keep it in view for a bugfix release (we can always postpone it later).

[jorisvandenbossche]

@TomAugspurger do you remember if the usage of take_1d instead of safe_sort was important performance-wise?

[jorisvandenbossche]

Alternatively, we could also (for short term fix), check if na_sentinel is specified to be something else as -1, and in that case always use the fall back of safe_sort.