Access Frame attributes instead of ColumnAccessor attributes when available #16652
Conversation
mroeschke
added
Python
Discovered in #16652, `DataFrame.iloc/loc.__setitem__` with a non-cupy type e.g. `"category"` failed because the indexing path unconditionally tries to `cupy.asarray` the value to be set which only accepts types recognized by cupy. We can skip this `asarray` if we have a numpy/pandas/cudf object Authors: - Matthew Roeschke (https://github.com/mroeschke) Approvers: - GALI PREM SAGAR (https://github.com/galipremsagar) URL: #16677
python/cudf/cudf/_lib/csv.pyx
Outdated
| @@ -273,8 +273,7 @@ def read_csv( | |||
| elif isinstance(dtype, abc.Collection): | |||
| for index, col_dtype in enumerate(dtype): | |||
| if isinstance(cudf.dtype(col_dtype), cudf.CategoricalDtype): | |||
| col_name = df._data.names[index] | |||
| df._data[col_name] = df._data[col_name].astype(col_dtype) | |||
| df.iloc[:, index] = df.iloc[:, index].astype(col_dtype) | |||
There was a problem hiding this comment.
Will this change not end up being expensive since we are touching the public dataframe API + typecasting a series which will be having the returning the index too?
There was a problem hiding this comment.
Yeah, this is quite a lot more expensive. We're replacing two dict lookups with quite a lot of work to determine how to do those two dict lookups.
There was a problem hiding this comment.
Yeah that's fair. I'll revert this change
There was a problem hiding this comment.
Some very small nits, in addition to @galipremsagar's comment.
python/cudf/cudf/_lib/csv.pyx
Outdated
| @@ -273,8 +273,7 @@ def read_csv( | |||
| elif isinstance(dtype, abc.Collection): | |||
| for index, col_dtype in enumerate(dtype): | |||
| if isinstance(cudf.dtype(col_dtype), cudf.CategoricalDtype): | |||
| col_name = df._data.names[index] | |||
| df._data[col_name] = df._data[col_name].astype(col_dtype) | |||
| df.iloc[:, index] = df.iloc[:, index].astype(col_dtype) | |||
There was a problem hiding this comment.
Yeah, this is quite a lot more expensive. We're replacing two dict lookups with quite a lot of work to determine how to do those two dict lookups.
python/cudf/cudf/core/frame.py
Outdated
| return zip(self._column_names, self._columns) | ||
|
|
||
| @property | ||
| def _dtypes(self) -> abc.Generator: |
There was a problem hiding this comment.
| def _dtypes(self) -> abc.Generator: | |
| def _dtypes(self) -> abc.Generator[tuple[Hashable, Dtype], None, None]: |
? can't remember what the type of col.dtype is
There was a problem hiding this comment.
Thanks. Yup it's Dtype
python/cudf/cudf/core/frame.py
Outdated
| @@ -75,8 +75,13 @@ def _columns(self) -> tuple[ColumnBase, ...]: | |||
| return self._data.columns | |||
|
|
|||
| @property | |||
| def _dtypes(self) -> abc.Iterable: | |||
| return zip(self._data.names, (col.dtype for col in self._data.columns)) | |||
| def _column_labels_and_values(self) -> abc.Iterable: | |||
There was a problem hiding this comment.
| def _column_labels_and_values(self) -> abc.Iterable: | |
| def _column_labels_and_values(self) -> abc.Iterable[tuple[Hashable, Dtype]]: |
?
There was a problem hiding this comment.
The second argument in the tuple should be ColumnBase, but thanks!
|
/merge |
…ilable (rapidsai#16652) There are some places where a public object like `DataFrame` or `Index` accesses a `ColumnAccessor` attribute when it's accessible in a shared subclass attribute instead (like `Frame`). In an effort to access the `ColumnAccessor` less, replaced usages of `._data.attribute` with a `Frame` specific attribute` Authors: - Matthew Roeschke (https://github.com/mroeschke) - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - GALI PREM SAGAR (https://github.com/galipremsagar) URL: rapidsai#16652
Description
There are some places where a public object like
DataFrameorIndexaccesses aColumnAccessorattribute when it's accessible in a shared subclass attribute instead (likeFrame).In an effort to access the
ColumnAccessorless, replaced usages of._data.attributewith aFramespecific attribute`Checklist