Skip to content

Migrate string slice APIs to pylibcudf - #15988

Merged
rapids-bot[bot] merged 18 commits into
NVIDIA:branch-24.08from
brandon-b-miller:pylibcudf-strings-slice
Jun 26, 2024
Merged

Migrate string slice APIs to pylibcudf#15988
rapids-bot[bot] merged 18 commits into
NVIDIA:branch-24.08from
brandon-b-miller:pylibcudf-strings-slice

Conversation

@brandon-b-miller

Copy link
Copy Markdown
Contributor

This PR introduces pylibcudf string slice APIs and migrates the cuDF cython to use them. Part of #15162

@brandon-b-miller
brandon-b-miller requested a review from a team as a code owner June 12, 2024 02:37
@github-actions github-actions Bot added Python Affects Python cuDF API. CMake CMake build issue pylibcudf Issues specific to the pylibcudf package labels Jun 12, 2024
@brandon-b-miller brandon-b-miller added feature request New feature or request non-breaking Non-breaking change labels Jun 12, 2024

@charlesbluca charlesbluca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @brandon-b-miller! Some small drive-by comments for now:

Comment thread python/cudf/cudf/_lib/pylibcudf/strings/slice.pyx
from cudf._lib.pylibcudf.libcudf.scalar.scalar_factories cimport (
make_fixed_width_scalar as cpp_make_fixed_width_scalar,
)
from cudf._lib.pylibcudf.libcudf.strings cimport substring as cpp_slice

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not blocking this PR, but wonder if it makes sense to align the libcudf/pylibcudf structure on either substring or slice rather than using both interchangeably (though don't have much context with strings submodules to know if this was an intentional choice)

Comment thread python/cudf/cudf/_lib/strings/substring.pyx
@brandon-b-miller

Copy link
Copy Markdown
Contributor Author

There's a failing test here that could be coming from libcudf - I get different answers for a string with just one space in it depending on if I use the column wise or scalar APIs:

import pyarrow as pa
import cudf._lib.pylibcudf as plc

col = plc.interop.from_arrow(
    pa.array(
        [
            " "
        ]
    )
)

slr_start = plc.interop.from_arrow(pa.scalar(-1))
slr_stop = plc.interop.from_arrow(pa.scalar(-1))

res = plc.strings.slice.slice_strings(col, slr_start, slr_stop)
print(plc.interop.to_arrow(res))

col_start = plc.interop.from_arrow(
    pa.array(
        [
            -1
        ]
    )
)

col_stop = plc.interop.from_arrow(
    pa.array(
        [
            -1
        ]
    )
)

res = plc.strings.slice.slice_strings(col, col_start, col_stop)

print(plc.interop.to_arrow(res))

This prints

[
  [
    ""
  ]
]
[
  [
    " "
  ]
]

Should we expect do get a different result for slice(" ", start=-1, stop=-1) depending on if the -1's come from a scalar or column?

@brandon-b-miller

Copy link
Copy Markdown
Contributor Author

Should we expect do get a different result for slice(" ", start=-1, stop=-1) depending on if the -1's come from a scalar or column?

cc maybe @davidwendt for the above

@davidwendt

davidwendt commented Jun 24, 2024

Copy link
Copy Markdown
Contributor

The scalar version does operate different from the columnar version when negative values are involved.
Documentation here does not describe this: https://docs.rapids.ai/api/libcudf/stable/group__strings__slice#gaa6f853a3b807391dadad69ad8727bac0

Negative values for the scalar version do a wrap around to be consistent with the pandas string slicing.
https://pandas.pydata.org/docs/reference/api/pandas.Series.str.slice.html
The columnar version does not support negative values in this way. You can pass negative values for the stops column to indicate to-the-end-of-the-string. Otherwise, out-of-bounds position values will result in an empty string.

@vyasr

vyasr commented Jun 24, 2024

Copy link
Copy Markdown
Contributor

Should we update the docs in this PR? That seems like the main action item if the test behavior is correct.

@brandon-b-miller

Copy link
Copy Markdown
Contributor Author

Thanks @davidwendt for the context I managed to handle the edge case in 78fe267. All tests should pass here now.

@wence- wence- left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some small queries

Comment thread python/cudf/cudf/_lib/pylibcudf/strings/slice.pyx
Comment thread python/cudf/cudf/_lib/pylibcudf/strings/slice.pyx
Comment thread python/cudf/cudf/_lib/pylibcudf/strings/slice.pyx
Comment thread python/cudf/cudf/_lib/pylibcudf/strings/slice.pyx

@wence- wence- left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks Brandon

@brandon-b-miller

Copy link
Copy Markdown
Contributor Author

/merge

@rapids-bot
rapids-bot Bot merged commit e0b8ab0 into NVIDIA:branch-24.08 Jun 26, 2024
@brandon-b-miller
brandon-b-miller deleted the pylibcudf-strings-slice branch June 26, 2024 23:07
rapids-bot Bot pushed a commit that referenced this pull request Jul 3, 2024
This PR plumbs the libcudf/pylibcudf `slice_strings` function through to cudf-polars. Depends on #15988

Authors:
  - https://github.com/brandon-b-miller
  - GALI PREM SAGAR (https://github.com/galipremsagar)

Approvers:
  - Lawrence Mitchell (https://github.com/wence-)

URL: #16082
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CMake CMake build issue feature request New feature or request non-breaking Non-breaking change pylibcudf Issues specific to the pylibcudf package Python Affects Python cuDF API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants