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

BUG: pandas.Series.unique() does not return correct unique values on \u string #45929

Open
2 of 3 tasks
cbhushan opened this issue Feb 10, 2022 · 4 comments
Open
2 of 3 tasks
Assignees
Labels
Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff Bug Strings String extension data type and string data

Comments

@cbhushan
Copy link

cbhushan commented Feb 10, 2022

Pandas version checks

  • I have checked that this issue has not already been reported.

  • I have confirmed this bug exists on the latest version of pandas.

  • I have confirmed this bug exists on the main branch of pandas.

Reproducible Example

import pandas as pd
import numpy as np

data_list = [
    '1 \udcd6a NY',
    '2 \udcd6b NY',
    '3 \ud800c NY',
    '4 \udcd6d NY',
    '5 \udcc3e NY',]

ss = pd.Series(data_list)
print(f'len(ss.unique()): {len(ss.unique())}')
print(f'len(ss.value_counts()): {len(ss.value_counts())}')
print(f'len(np.unique(data_list): {len(np.unique(data_list))}')
print(f'len(set(data_list)): {len(set(data_list))}')
print(f'ss.is_unique: {ss.is_unique}')

Issue Description

pandas.Series.unique() does not return correct unique values when working with non-standard strings. In above example data_list clearly has 5 unique elements but ss.unique() returns an array of length-1. ss.is_unique is also wrong (it should be True). Surprising, ss.value_counts() seems to work . In contrast, np.unique and set() works as expected.

Above is a small reproducible example, I came across this issue while working with filenames with non-ascii characters on a Linux system.

See Output below:

len(ss.unique()): 1
len(ss.value_counts()): 5
len(np.unique(data_list): 5
len(set(data_list)): 5
ss.is_unique: False

Expected Behavior

All of the length should equal 5 and ss.is_unique should return True.

Installed Versions

pd.show_versions()

INSTALLED VERSIONS

commit : bb1f651
python : 3.8.6.final.0
python-bits : 64
OS : Linux
OS-release : 4.15.0-154-generic
Version : #161-Ubuntu SMP Fri Jul 30 13:04:17 UTC 2021
machine : x86_64
processor : x86_64
byteorder : little
LC_ALL : None
LANG : en_US.UTF-8
LOCALE : en_US.UTF-8

pandas : 1.4.0
numpy : 1.22.2
pytz : 2021.3
dateutil : 2.8.2
pip : 22.0.3
setuptools : 51.1.1
Cython : None
pytest : None
hypothesis : None
sphinx : None
blosc : None
feather : None
xlsxwriter : None
lxml.etree : None
html5lib : None
pymysql : None
psycopg2 : None
jinja2 : None
IPython : None
pandas_datareader: None
bs4 : None
bottleneck : None
fastparquet : None
fsspec : None
gcsfs : None
matplotlib : None
numba : None
numexpr : None
odfpy : None
openpyxl : None
pandas_gbq : None
pyarrow : None
pyreadstat : None
pyxlsb : None
s3fs : None
scipy : None
sqlalchemy : None
tables : None
tabulate : None
xarray : None
xlrd : None
xlwt : None
zstandard : None

@cbhushan cbhushan added Bug Needs Triage Issue that has not been reviewed by a pandas team member labels Feb 10, 2022
@mroeschke mroeschke added Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff Strings String extension data type and string data and removed Needs Triage Issue that has not been reviewed by a pandas team member labels Feb 11, 2022
@mroeschke mroeschke changed the title BUG: pandas.Series.unique() does not return correct unique values. BUG: pandas.Series.unique() does not return correct unique values on \u string Feb 11, 2022
@jamesholcombe
Copy link
Contributor

take

@jamesholcombe
Copy link
Contributor

Okay, I have done some intial investigation as to what is happening.

I know where and what is happening, but not the why!

In short, something is happening wrong in either the assignment or retrieval of the "u string" elements to the vector vecs which is defined here:

vecs = <const char **>malloc(n * sizeof(char *))

We then loop through each string value, obtain the c string (note that we hit the unicode enode error), and assign it to the vecs at the given index.

for i in range(n):
val = values[i]
if (ignore_na
and (not isinstance(val, str)
or (use_na_value and val == na_value))):
# if missing values do not count as unique values (i.e. if
# ignore_na is True), we can skip the actual value, and
# replace the label with na_sentinel directly
labels[i] = na_sentinel
else:
# if ignore_na is False, we also stringify NaN/None/etc.
try:
v = get_c_string(<str>val)
except UnicodeEncodeError:
v = get_c_string(<str>repr(val))
vecs[i] = v

When we set this value at the loop index, it seemingly overwrites the other values of the vector.

For each loop, when we try and retrieve the values from the vector that have been previously set:

                #unchanged code
                try:
                    v = get_c_string(<str>val)
                except UnicodeEncodeError:
                    v = get_c_string(<str>repr(val))
                vecs[i] = v
               
                #added code to show previously set values. 
                for x in range(i + 1):
                    print("loop num",x, "\n")
                    for y in range(i + 1):
                        print(y,": ", vecs[y])

Output is something like this:

loop num 0 

0 :  b"'1 \\udcd6a NY'"
loop num 0 

0 :  b"'2 \\udcd6b NY'"
1 :  b"'2 \\udcd6b NY'"
loop num 1 

0 :  b"'2 \\udcd6b NY'"
1 :  b"'2 \\udcd6b NY'"
loop num 0 

0 :  b"'3 \\ud800c NY'"
1 :  b"'3 \\ud800c NY'"
2 :  b"'3 \\ud800c NY'"

#etc

I have checked with other string series and what we should see is something like:

loop num 0 

0 :  b"'1 \\udcd6a NY'"
loop num 0 

0 :   b"'1 \\udcd6a NY'"
1 :  b"'2 \\udcd6b NY'"
loop num 1 

0 :  b"'1 \\udcd6a NY'"
1 :  b"'2 \\udcd6b NY'"
loop num 0 

0 :   b"'1 \\udcd6a NY'"
1 :  b"'2 \\udcd6b NY'"
2 :  b"'3 \\ud800c NY'"

@mroeschke I don't have a great knowledge of C pointers so any guidance is appreciated!

@MichaelTiemannOSC
Copy link
Contributor

Greetings. I spent 10 years working on C and C++ compilers, so have some distant memories of pointer issues.

I have been on my own merry chase as to why on single test case in my pull request is failing (Ubuntu 3.9 unit tests) and not elsewhere (Ubuntu 3.10 and 3.11 Unit tests, Windows 3.9-3.11 Unit tests, MacOS 3.9-3.11 Unit Tests, python-dev, etc). The failing Unit test run is here: https://github.com/pandas-dev/pandas/actions/runs/6516917739/job/17700916929

This issue looks like it might be related, especially because once I added your print statements (modified a small amount), I was able to reproduce the failure condition in my test case (base/test_unique.py "bad_unicode") that slips through without such statements. And it fails on Python 3.11, not only an obscure Python 3.9.

To warm up the conversation, https://docs.python.org/3/c-api/unicode.html#c.PyUnicode_AsUTF8AndSize (which is used by get_c_string) states

Return a pointer to the UTF-8 encoding of the Unicode object, and store the size of the encoded representation (in bytes) in size. The size argument can be NULL; in this case no size will be stored. The returned buffer always has an extra null byte appended (not included in size), regardless of whether there are any other null code points.

In the case of an error, NULL is returned with an exception set and no size is stored.

This caches the UTF-8 representation of the string in the Unicode object, and subsequent calls will return a pointer to the same buffer. The caller is not responsible for deallocating the buffer. The buffer is deallocated and pointers to it become invalid when the Unicode object is garbage collected.

I think I can show that <str>repr(val) is a candidate for premature garbage collection, and will report back if it does, in fact, fix my own Heisenbug.

Thanks for getting the ball rolling!

@MichaelTiemannOSC
Copy link
Contributor

Here's the commit that has the change that results in 5 unique strings, not the erroneous 1:

dabaf6f

It's too late tonight, but I'll make it a proper pull request tmw.

>>> import pandas as pd
>>> import numpy as np
>>> 
>>> data_list = [
...     '1 \udcd6a NY',
...     '2 \udcd6b NY',
...     '3 \ud800c NY',
...     '4 \udcd6d NY',
...     '5 \udcc3e NY',]
>>> 
>>> ss = pd.Series(data_list)
>>> print(f'len(ss.unique()): {len(ss.unique())}')
len(ss.unique()): 5
>>> print(f'len(ss.value_counts()): {len(ss.value_counts())}')
len(ss.value_counts()): 5
>>> print(f'len(np.unique(data_list): {len(np.unique(data_list))}')
len(np.unique(data_list): 5
>>> print(f'len(set(data_list)): {len(set(data_list))}')
len(set(data_list)): 5
>>> print(f'ss.is_unique: {ss.is_unique}')
ss.is_unique: True

MichaelTiemannOSC added a commit to MichaelTiemannOSC/pandas that referenced this issue Oct 15, 2023
When processing an invalid Unicode string, the exception handler for UnicodeEncodeError called `get_c_string` with an ephemeral repr value that could be garbage-collected the next time an exception was raised.  Issue pandas-dev#45929 demonstrates the problem.

This commit fixes the problem by retaining a Python reference to the repr value that underlies the C string until after all `values` are processed.

Wisdom from StackOverflow suggests that there's very small performance difference between pre-allocating the array vs. append if indeed we do need to fill it all the way, but because we only need references on exceptions, we expect that in the usual case we will append very few elements, making it faster than pre-allocation.

Signed-off-by: Michael Tiemann <[email protected]>
MichaelTiemannOSC added a commit to MichaelTiemannOSC/pandas that referenced this issue Oct 15, 2023
The `single_cpu` attribute for `test_unique_bad_unicode` was likely an attempt to cover over the underlying bug fixed with this commit.  We can now run this test in the usual fashion.

Added a test case for the problem reported in 45929.

Signed-off-by: Michael Tiemann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff Bug Strings String extension data type and string data
Projects
None yet
4 participants