Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Factorize don't preserve na_sentinel values when sort is specified #25409

Closed
rahulnair23 opened this issue Feb 22, 2019 · 7 comments

Comments

@rahulnair23
Copy link

commented Feb 22, 2019

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

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

@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 comment has been minimized.

Copy link
Member

commented Feb 22, 2019

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 gfyoung removed this from the 0.24.2 milestone Feb 23, 2019

@gfyoung

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

(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 jorisvandenbossche added this to the 0.24.2 milestone Feb 23, 2019

@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

@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

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

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

@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Mar 4, 2019

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.

@TomAugspurger

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

I don't recall, sorry.
https://github.com/pandas-dev/pandas/pull/19938/files#r173567099 may be suggestive though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.