Skip to content

Fix string to double conversion and row equivalent comparison - #7410

Merged
rapids-bot[bot] merged 24 commits into
NVIDIA:branch-0.19from
ttnghia:branch-0.19-issue-5225
Mar 2, 2021
Merged

Fix string to double conversion and row equivalent comparison#7410
rapids-bot[bot] merged 24 commits into
NVIDIA:branch-0.19from
ttnghia:branch-0.19-issue-5225

Conversation

@ttnghia

@ttnghia ttnghia commented Feb 18, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

This PR fixes #5225 and fixes #5731 along with several improvements. In particular:

  • Fixes the function stod in convert_floats.cu, which incorrectly uses max_mantissa, thus produces incorrect results as mentioned in the issue [BUG] discrepency in casting String to double #5225. Note that this PR still cannot produce perfectly matched results with std::atof, since handling float values near inf is very difficult.
  • Fixes the class corresponding_rows_not_equivalent which didn't handle inf and nan, thus it returned "equivalent" when comparing a valid float number with inf.
  • Adds a test case for those fixes.
  • Rewrite test cases for string=>float coversion to use std::atof results for comparison.

@ttnghia
ttnghia requested review from a team as code owners February 18, 2021 17:54
@github-actions github-actions Bot added CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. labels Feb 18, 2021
@ttnghia ttnghia added 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond bug Something isn't working improvement Improvement / enhancement to an existing function strings strings issues (C++ and Python) tests Unit testing for project and removed CMake CMake build issue labels Feb 18, 2021
@ttnghia
Merge remote-tracking branch 'origin/branch-0.19' into branch-0.19-is…
7778f85 
…sue-5225

# Conflicts:
#	cpp/benchmarks/CMakeLists.txt
@github-actions github-actions Bot added the CMake CMake build issue label Feb 18, 2021
@ttnghia ttnghia removed the CMake CMake build issue label Feb 18, 2021
@ttnghia ttnghia changed the title Fix string to double conversion Fix string to double conversion and row equivalent comparison Feb 18, 2021
@github-actions github-actions Bot added the CMake CMake build issue label Feb 18, 2021
@davidwendt
davidwendt self-requested a review February 18, 2021 18:31
davidwendt
davidwendt requested changes Feb 18, 2021
View reviewed changes

@davidwendt davidwendt 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.

This is excellent.
Looks like you reformatted the CMakeLists.txt file.
You should change that back please.

Comment thread cpp/tests/strings/floats_tests.cpp
jrhemstad
jrhemstad reviewed Feb 18, 2021
View reviewed changes
Comment thread cpp/tests/utilities/column_utilities.cu Outdated
@vuule vuule removed the improvement Improvement / enhancement to an existing function label Feb 18, 2021
kkraus14
kkraus14 approved these changes Feb 18, 2021
View reviewed changes

@kkraus14 kkraus14 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.

cmake lgtm

@revans2 revans2 mentioned this pull request Feb 18, 2021
Open
davidwendt
davidwendt reviewed Feb 22, 2021
View reviewed changes
Comment thread cpp/benchmarks/string/convert_floats_benchmark.cpp Outdated
davidwendt
davidwendt reviewed Feb 22, 2021
View reviewed changes
Comment thread cpp/tests/utilities/column_utilities.cu Outdated
@revans2 revans2 mentioned this pull request Feb 22, 2021
Merged
@ttnghia
Merge remote-tracking branch 'origin/branch-0.19' into branch-0.19-is…
6b1af3b 
…sue-5225

# Conflicts:
#	cpp/benchmarks/CMakeLists.txt
@ttnghia ttnghia added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Feb 23, 2021
davidwendt
davidwendt requested changes Feb 23, 2021
View reviewed changes
Comment thread cpp/benchmarks/string/convert_floats_benchmark.cpp Outdated
@ttnghia @davidwendt
Update header format for cpp/benchmarks/string/convert_floats_benchma…
3a7c52e 
…rk.cpp

Co-authored-by: David <45795991+davidwendt@users.noreply.github.com>
davidwendt
davidwendt approved these changes Feb 23, 2021
View reviewed changes

@davidwendt davidwendt 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.

Looks good. Thanks for doing this.

vuule
vuule requested changes Feb 26, 2021
View reviewed changes

@vuule vuule 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.

Couple suggestions, mostly related to data generation.

Comment thread cpp/src/strings/convert/convert_floats.cu
Comment thread cpp/benchmarks/string/convert_floats_benchmark.cpp Outdated
Comment thread cpp/benchmarks/string/convert_floats_benchmark.cpp Outdated
Comment thread cpp/benchmarks/string/convert_floats_benchmark.cpp Outdated
Comment thread cpp/benchmarks/string/convert_floats_benchmark.cpp Outdated
@revans2 revans2 mentioned this pull request Mar 1, 2021
Merged
@vuule
vuule self-requested a review March 1, 2021 22:09
@ttnghia

ttnghia commented Mar 2, 2021

Copy link
Copy Markdown
Contributor Author

@gpucibot merge.

@harrism

harrism commented Mar 2, 2021

Copy link
Copy Markdown
Contributor

@ttnghia you can't merge if CI is not passing...

@harrism

harrism commented Mar 2, 2021

Copy link
Copy Markdown
Contributor

@gpucibot rerun tests

@ttnghia

ttnghia commented Mar 2, 2021

Copy link
Copy Markdown
Contributor Author

I see. Thanks Mark.
It seems that there are some issues with CI accounts so the server could not run.

@ttnghia
Merge branch 'branch-0.19' into branch-0.19-issue-5225
da19661 
# Conflicts:
#	cpp/benchmarks/CMakeLists.txt
@ttnghia

ttnghia commented Mar 2, 2021

Copy link
Copy Markdown
Contributor Author

@gpucibot merge.

@kkraus14

kkraus14 commented Mar 2, 2021

Copy link
Copy Markdown
Contributor

@gpucibot merge

@rapids-bot
rapids-bot Bot merged commit f8fa481 into NVIDIA:branch-0.19 Mar 2, 2021
rapids-bot Bot pushed a commit that referenced this pull request Mar 2, 2021
@revans2
Fix java float/double parsing tests (#7473)
cf4c54d 
This is a part of #7410 and depends on it.  The java tests when they were first written, for what ever reason, did not use the correct string representation for several values including double max.  They ended up testing that the code was broken, which is not the right thing to do.  This updates those to have the correct values, but also has a comment about why the ideal value to test for overflow into `Inf` and `-Inf` is not ideal, but is a work around for small issues with the parsing code still.

Authors:
  - Robert (Bobby) Evans (@revans2)
  - Nghia Truong (@ttnghia)

Approvers:
  - Jason Lowe (@jlowe)

URL: #7473
@ttnghia
ttnghia deleted the branch-0.19-issue-5225 branch March 11, 2021 18:33
@ttnghia ttnghia self-assigned this Apr 25, 2021
@GregoryKimball GregoryKimball mentioned this pull request Apr 5, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@revans2 revans2 revans2 approved these changes
@jrhemstad jrhemstad jrhemstad left review comments
@vuule vuule vuule approved these changes
@davidwendt davidwendt davidwendt approved these changes
+1 more reviewer
@kkraus14 kkraus14 kkraus14 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@ttnghia ttnghia

Labels

3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond 5 - Ready to Merge Testing and reviews complete, ready to merge bug Something isn't working CMake CMake build issue libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change strings strings issues (C++ and Python) tests Unit testing for project

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] expect_columns_equivalent handles infinity incorrectly. [BUG] discrepency in casting String to double

7 participants

@ttnghia @harrism @kkraus14 @revans2 @jrhemstad @vuule @davidwendt