Skip to content

BUG: read_csv type hint for na_values should include non-iterables as valid arguments #53813

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

Closed
3 tasks done
tpaxman opened this issue Jun 23, 2023 · 3 comments · Fixed by #54486
Closed
3 tasks done
Labels
Bug IO CSV read_csv, to_csv Needs Triage Issue that has not been reviewed by a pandas team member

Comments

@tpaxman
Copy link
Contributor

tpaxman commented Jun 23, 2023

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
df = pd.DataFrame({'a': [1,2,3], 'b': list('hij'), 'c': ['A', 'N/A', 'BBB']})
df.to_csv('test.csv', index=False)

# passing scalar argument
pd.read_csv('test.csv', na_values=2)
# output:
#       a  b    c
#  0  1.0  h    A
#  1  NaN  i  NaN
#  2  3.0  j  BBB

# passing string argument
pd.read_csv('test.csv', na_values='BBB')
# output:
#     a  b    c
#  0  1  h    A
#  1  2  i  NaN
#  2  3  j  NaN

# passing list of scalar and string as argument
>>> pd.read_csv('test.csv', na_values=[2, 'BBB'])
#       a  b    c
#  0  1.0  h    A
#  1  NaN  i  NaN
#  2  3.0  j  NaN

Issue Description

The description of valid types in the read_csv docstring under na_values shows:

scalar, str, list-like, or dict, optional

However, the type signature implies that only list-like or dict are valid (not scalar or str):

Sequence[str] | Mapping[str, Sequence[str]] | None = None

It appears that the function does actually accept scalar or str (i.e. non-list-list, non-iterable) values as evidenced by the example above, meaning the description is accurate and the type hint needs to be updated to match. The main inconsistencies with the type hint are the following:

  • No indication of scalar or str being a valid argument
  • The sequence of str is too specific since a list containing str and scalar appears to be valid (e.g., [2, 'BBB'] seems to work).
  • The mapping also appears too specific, since the keys correspond to column names which are not always str (could be any Hashable) and the values in the mapping are too specific for the same reasons listed in the previous bullet.

Note: this was one of a few items I mentioned in #53763 but I thought it might need its own issue since it affects more than just the docstring. If it should have still been labelled 'DOC', please feel free to update.

Expected Behavior

The behavior seems to be correct, but the type hint should match, perhaps by updating to something like this:

Hashable | Iterable[Hashable] | Mapping[Hashable, Iterable[Hashable]] | None = None (assuming Hashable is the correct type to cover 'scalar or str` as shown in the description but perhaps someone more familiar could confirm this).

Installed Versions

/home/tpaxman/mambaforge/lib/python3.10/site-packages/_distutils_hack/__init__.py:33: UserWarning: Setuptools is replacing distutils. warnings.warn("Setuptools is replacing distutils.")

INSTALLED VERSIONS

commit : 965ceca
python : 3.10.10.final.0
python-bits : 64
OS : Linux
OS-release : 5.15.90.1-microsoft-standard-WSL2
Version : #1 SMP Fri Jan 27 02:56:13 UTC 2023
machine : x86_64
processor : x86_64
byteorder : little
LC_ALL : None
LANG : C.UTF-8
LOCALE : en_US.UTF-8

pandas : 2.0.2
numpy : 1.25.0
pytz : 2023.3
dateutil : 2.8.2
setuptools : 65.6.3
pip : 23.0.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
brotli :
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
snappy : None
sqlalchemy : None
tables : None
tabulate : None
xarray : None
xlrd : None
zstandard : 0.19.0
tzdata : 2023.3
qtpy : None
pyqt5 : None

@tpaxman tpaxman added Bug Needs Triage Issue that has not been reviewed by a pandas team member labels Jun 23, 2023
@twoertwein
Copy link
Member

However, the type signature implies that only list-like or dict are valid (not scalar or str):

Sequence[str] | Mapping[str, Sequence[str]] | None = None

Sequence[str] includes str

It appears that the function does actually accept scalar or str

If that is the intended behavior (I don't know), it would be good to update the doc-string and the type annotations.

@tpaxman
Copy link
Contributor Author

tpaxman commented Jun 25, 2023

Sequence[str] includes str

Touché! I didn't think about that.

I guess then the question would be how to allow non-string values to be passed as well. Seems like having a generalized type annotation that refers to values as Hashables, which I think represents the set of acceptable values (but now I'm trying to think whether there might be exceptions too...).

But as you said, I guess it will be important to verify that the behaviour is intended. I think it probably is as I think it makes sense to be defined this way, but I don't know for sure!

@topper-123
Copy link
Contributor

You could check the tests for read_csv/na_values to see if we test for non-string values for na_values.

IMO, if the check for na_values is done after reading in the data into a Dataframe, it makes sense it should accept non-string scalars also, while if the checks are against the read-in string values, it probably makes most sense to raise a TypeError if the scalar in na_values is a non-string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug IO CSV read_csv, to_csv Needs Triage Issue that has not been reviewed by a pandas team member
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants