Skip to content
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

Jump to bottom

Add istitle to string UDFs #11738

Merged
merged 7 commits into from
Sep 27, 2022
Merged

Add istitle to string UDFs #11738

merged 7 commits into from
Sep 27, 2022

Conversation

brandon-b-miller
Copy link
Contributor

This PR adds support for the use of thestr.istitle() method within udfs for apply.

@brandon-b-miller brandon-b-miller added feature request New feature or request Python Affects Python cuDF API. non-breaking Non-breaking change labels Sep 21, 2022
@brandon-b-miller brandon-b-miller requested a review from a team as a code owner September 21, 2022 16:38
@codecov
Copy link

codecov bot commented Sep 21, 2022 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.10@6131bd6). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head fff1916 differs from pull request most recent head d46c2f9. Consider uploading reports for the commit d46c2f9 to get more accurate results

Additional details and impacted files 
@@               Coverage Diff               @@
##             branch-22.10   #11738   +/-   ##
===============================================
  Coverage                ?   87.52%           
===============================================
  Files                   ?      133           
  Lines                   ?    21795           
  Branches                ?        0           
===============================================
  Hits                    ?    19076           
  Misses                  ?     2719           
  Partials                ?        0

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@GregoryKimball GregoryKimball added this to the UDF Enhancements milestone Sep 21, 2022
vyasr
vyasr requested changes Sep 26, 2022
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Small suggestions

python/strings_udf/cpp/include/cudf/strings/udf/char_types.cuh Outdated Show resolved Hide resolved
python/strings_udf/cpp/include/cudf/strings/udf/char_types.cuh Outdated
__device__ inline bool is_title(cudf::strings::detail::character_flags_table_type* flags_table,
string_view d_str)
{
bool valid = false; // requires one or more cased characters
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a test case that covers the branch where we start with valid and never enter the if statement below? I assume a numeric like "123" or a symbolic string like "^#(" would do the trick.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure whether it's best to test in the string UDF tests, the cudf tests, or both; up to you.

python/strings_udf/cpp/include/cudf/strings/udf/char_types.cuh Show resolved Hide resolved
vyasr
vyasr approved these changes Sep 27, 2022
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

I merged my one minor suggestion after I saw that you added tests and am approving. Feel free to merge when you think it's ready.

@brandon-b-miller
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit a270ae6 into rapidsai:branch-22.10 Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwendt davidwendt davidwendt approved these changes

@bdice bdice bdice left review comments

@vyasr vyasr vyasr approved these changes

@skirui-source skirui-source Awaiting requested review from skirui-source skirui-source is a code owner automatically assigned from rapidsai/cudf-python-codeowners

Assignees
No one assigned
Labels
feature request New feature or request non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Milestone
UDF Enhancements
Development

Successfully merging this pull request may close these issues.
5 participants
@brandon-b-miller @vyasr @bdice @davidwendt @GregoryKimball