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

Update check for inf/nan strings in libcudf float conversion to ignore case #9694

Merged
merged 14 commits into from
Nov 30, 2021
Merged

Update check for inf/nan strings in libcudf float conversion to ignore case #9694

merged 14 commits into from
Nov 30, 2021

Conversation

davidwendt
Copy link
Contributor

@davidwendt davidwendt commented Nov 16, 2021 •
edited
Loading

Reference https://github.com/rapidsai/cudf/pull/9613/files#r743579126

Add support to ignore case for strings INF, INFINITY and NAN to cudf::strings::is_float and cudf::strings::to_float for consistency with https://en.cppreference.com/w/cpp/string/basic_string/stof

Also, remove the expensive replace call in the cudf before calling this from Python.

@davidwendt davidwendt added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. strings strings issues (C++ and Python) improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 16, 2021
@davidwendt davidwendt self-assigned this Nov 16, 2021
@github-actions github-actions bot added the Python Affects Python cuDF API. label Nov 16, 2021
@codecov
Copy link

codecov bot commented Nov 16, 2021 •
edited
Loading

Codecov Report

Merging #9694 (828da0a) into branch-22.02 (967a333) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head 828da0a differs from pull request most recent head e89b760. Consider uploading reports for the commit e89b760 to get more accurate results
Impacted file tree graph

@@               Coverage Diff                @@
##           branch-22.02    #9694      +/-   ##
================================================
- Coverage         10.49%   10.47%   -0.02%     
================================================
  Files               119      119              
  Lines             20305    20336      +31     
================================================
  Hits               2130     2130              
- Misses            18175    18206      +31
Impacted Files Coverage Δ
python/cudf/cudf/__init__.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/column.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/string.py 0.00% <ø> (ø)
python/cudf/cudf/core/frame.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/index.py 0.00% <ø> (ø)
python/cudf/cudf/core/indexed_frame.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/multiindex.py 0.00% <0.00%> (ø)
python/cudf/cudf/utils/utils.py 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 27b7190...e89b760. Read the comment docs.

@davidwendt davidwendt changed the title Add check for upper-case inf/nan in libcudf float conversion Update check for inf/nan strings in libcudf float conversion to ignore case Nov 16, 2021
@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Nov 17, 2021
@davidwendt davidwendt marked this pull request as ready for review November 17, 2021 18:16
@davidwendt davidwendt requested review from a team as code owners November 17, 2021 18:16
robertmaynard
robertmaynard reviewed Nov 17, 2021
View reviewed changes
Copy link
Contributor

@robertmaynard robertmaynard left a comment •
edited
Loading

Choose a reason for hiding this comment

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

One small issue ( previous comment was before I found the logic that stripped - or + from before Nan/Inf

cpp/include/cudf/strings/string.cuh Show resolved Hide resolved
@revans2 revans2 mentioned this pull request Nov 18, 2021
Closed
@davidwendt
Copy link
Contributor Author

@gpucibot merge

@codereport codereport removed their request for review November 23, 2021 18:09
@davidwendt davidwendt added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Nov 29, 2021
@github-actions github-actions bot added the Java Affects Java cuDF API. label Nov 30, 2021
@davidwendt davidwendt added 3 - Ready for Review Ready for review by team and removed 5 - Ready to Merge Testing and reviews complete, ready to merge labels Nov 30, 2021
@davidwendt davidwendt requested review from a team and removed request for a team and charlesbluca November 30, 2021 18:41
@davidwendt davidwendt requested a review from a team November 30, 2021 18:44
@rapids-bot rapids-bot bot merged commit 69d5765 into rapidsai:branch-22.02 Nov 30, 2021
@davidwendt davidwendt deleted the uppercase-inf-nan branch November 30, 2021 22:53
@pxLi pxLi mentioned this pull request Dec 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrhemstad jrhemstad jrhemstad left review comments

@robertmaynard robertmaynard robertmaynard approved these changes

@jlowe jlowe jlowe approved these changes

@ttnghia ttnghia ttnghia approved these changes

@galipremsagar galipremsagar galipremsagar approved these changes

Assignees

@davidwendt davidwendt

Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Python Affects Python cuDF API. strings strings issues (C++ and Python)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@davidwendt @robertmaynard @jlowe @ttnghia @galipremsagar @jrhemstad