Skip to content

BUG: interchange categorical_column_to_series() should not accept only PandasColumn #52763

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Apr 19, 2023
+25 −4
Merged

BUG: interchange categorical_column_to_series() should not accept only PandasColumn #52763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a0d3b44
round-trip categorical pyarrow
Apr 18, 2023
0e176b5
fixup for jobs where pyarrow is not present
Apr 18, 2023
e709c4c
use importorskip
Apr 18, 2023
fbef6c6
set minimum pyarrow version
Apr 19, 2023
936e6cd
just type: ignore
Apr 19, 2023
2cc5315
raise notimplementederror
Apr 19, 2023
6663714
raise notimplementederror
Apr 19, 2023
1d27e7d
Update pandas/core/interchange/from_dataframe.py
MarcoGorelli Apr 19, 2023
24a9157
Merge remote-tracking branch 'upstream/main' into categorical-xch
Apr 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/source/whatsnew/v2.0.1.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ Bug fixes
- Bug in :attr:`Series.dt.days` that would overflow ``int32`` number of days (:issue:`52391`)
- Bug in :class:`arrays.DatetimeArray` constructor returning an incorrect unit when passed a non-nanosecond numpy datetime array (:issue:`52555`)
- Bug in :func:`Series.median` with :class:`ArrowDtype` returning an approximate median (:issue:`52679`)
- Bug in :func:`api.interchange.from_dataframe` was unnecessarily raising on-categorical dtypes (:issue:`49889`)
- Bug in :func:`pandas.testing.assert_series_equal` where ``check_dtype=False`` would still raise for datetime or timedelta types with different resolutions (:issue:`52449`)
- Bug in :func:`read_csv` casting PyArrow datetimes to NumPy when ``dtype_backend="pyarrow"`` and ``parse_dates`` is set causing a performance bottleneck in the process (:issue:`52546`)
- Bug in :func:`to_datetime` and :func:`to_timedelta` when trying to convert numeric data with a :class:`ArrowDtype` (:issue:`52425`)
Expand Down
13 changes: 9 additions & 4 deletions pandas/core/interchange/from_dataframe.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import numpy as np

import pandas as pd
from pandas.core.interchange.column import PandasColumn
from pandas.core.interchange.dataframe_protocol import (
Buffer,
Column,
Expand Down Expand Up @@ -181,9 +180,15 @@ def categorical_column_to_series(col: Column) -> tuple[pd.Series, Any]:
raise NotImplementedError("Non-dictionary categoricals not supported yet")

cat_column = categorical["categories"]
# for mypy/pyright
assert isinstance(cat_column, PandasColumn), "categories must be a PandasColumn"
categories = np.array(cat_column._col)
if hasattr(cat_column, "_col"):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for using this attribute? (that also seems like relying on and exposing some internal detail)
Haven't looked in detail, but my understanding is that this object is just a protocol Column object, and we already have all the code to convert that to a pandas object?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, this was already there - looks like it was introduced in https://github.com/pandas-dev/pandas/pull/47886/files

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I know it was already there, but IMO the "proper" fix for the reported issue is to stop relying on this detail.
Because the the test you added only "happens" to work because pyarrow's Column object also uses _col to store the original array; if we would rename that internal property (and I would say it's actually a confusing name in hindsight, since it's an array ;)), the test would start failing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, agreed

# Item "Column" of "Optional[Column]" has no attribute "_col"
# Item "None" of "Optional[Column]" has no attribute "_col"
categories = np.array(cat_column._col) # type: ignore[union-attr]
else:
raise NotImplementedError(
"Interchanging categorical columns isn't supported yet, and our "
"fallback of using the `col._col` attribute (a ndarray) failed."
)
buffers = col.get_buffers()

codes_buff, codes_dtype = buffers["data"]
Expand Down
15 changes: 15 additions & 0 deletions pandas/tests/interchange/test_impl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,21 @@ def test_categorical_dtype(data):
tm.assert_frame_equal(df, from_dataframe(df.__dataframe__()))


def test_categorical_pyarrow():
# GH 49889
pa = pytest.importorskip("pyarrow", "11.0.0")

arr = ["Mon", "Tue", "Mon", "Wed", "Mon", "Thu", "Fri", "Sat", "Sun"]
table = pa.table({"weekday": pa.array(arr).dictionary_encode()})
exchange_df = table.__dataframe__()
result = from_dataframe(exchange_df)
weekday = pd.Categorical(
arr, categories=["Mon", "Tue", "Wed", "Thu", "Fri", "Sat", "Sun"]
)
expected = pd.DataFrame({"weekday": weekday})
tm.assert_frame_equal(result, expected)


@pytest.mark.parametrize(
"data", [int_data, uint_data, float_data, bool_data, datetime_data]
)
Expand Down