NVIDIA / NVIDIA/cudf

[FEA] Use table_view interface in scatter-by-foo API in CUDF

Open
#13,526 3 comments 0 reactions 0 assignees View on GitHub
feature request Performance Python
Dominant language
C++
Stars
9.8k
Forks
1.1k
Avg merge
3d 6m
Merged PRs (30d)
278

Description

Both the scatter-like and gather-like Cython interfaces to libcudf's copying API accept a list of source and target columns as a "table". This feature is, however, only _used_ in the front-facing API by the gather-like calls.

For scatter-like calls on a dataframe, the final result is usually achieved by creating series for each column to scatter to/from and calling the singleton scatter on the series before reconstructing. This is more wasteful than it needs to be and we should probably instead just create the list of columns we wish to scatter and scatter that instead.

For example, the low-level Cython interface to scatter accepts a list of source and target columns:

https://github.com/rapidsai/cudf/blob/8055c2db6f80286b64d36f3927510bcf2e0eec02/python/cudf/cudf/_lib/copying.pyx#L240

However, the only caller in the python codebase is `ColumnBase._scatter_by_column` https://github.com/rapidsai/cudf/blob/8055c2db6f80286b64d36f3927510bcf2e0eec02/python/cudf/cudf/core/column/column.py#L647-L682 where we only have a single column to hand and hence can only pass one column.

This is usage is a consequence of `__setitem__` on `DataFrame`s ultimately being implemented by spinning over the columns (as series) and calling setitem on each series in turn. For example, `DataFrameIlocIndexer.__setitem__` has this code (when setting columns using a dataframe as the rvalue):

https://github.com/rapidsai/cudf/blob/8055c2db6f80286b64d36f3927510bcf2e0eec02/python/cudf/cudf/core/dataframe.py#L466-L477

This calls (on line 474) `Series.__setitem__` on each which either goes to `series.iloc.__setitem__` or `series.loc.__setitem__`, and eventually devolves to `ColumnBase.__setitem__`.

(Aside, this is actually a bug, since `iloc.__setitem__` should not do index alignment which `loc` will do).

Ignoring that bug issue, this code is somewhat wasteful in a number of ways:

1. we eventually throw away the `Series` object when reconstructing the dataframe, so should just operate on columns
2. at this point in `DataFrame.iloc.__setitem__` we _already know_ everything about the key (and hence which low-level libcudf operation to dispatch to) so we should do that rather than going through the front door of `Series.__setitem__` which will endeavour to rediscover everything we already know.
3. we call into libcudf n times (once for each column), rather than once with n columns, which negates the ability to use the stream parallelism that is implemented in the `scatter` code in libcudf

To fix this, I propose that `DataFrame` and `Series` should both get a new set of methods. Here's a first pass at what these should be called/their interface. These are internal and should do no argument normalisation or bounds-checking (it being the responsibility of the caller to sanitise appropriately), this includes kind/dtype casting as appropriate of the source and target.

- `_scatter_by_indices(self, source_columns, index_column, *, keep_index=True)`
- `_scatter_by_mask(self, source_columns, mask_column, *, keep_index=True)`
- `_scatter_by_slice(self, source_columns, slice, *, keep_index=True)`

Since there is also a broadcasting `scatter_from_scalar` operation in libcudf we'll probably need to support that too. This should be done with a `value_type` tag in the interface (rather than isinstancing on the `source_columns`) since, again, the caller must have already determined this information.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.