Skip to content

Set +/-infinity as the identity values for floating-point numbers in device operators min and max - #11357

Merged
rapids-bot[bot] merged 12 commits into
NVIDIA:branch-22.08from
ttnghia:fix_min_max
Jul 28, 2022
Merged

Set +/-infinity as the identity values for floating-point numbers in device operators min and max#11357
rapids-bot[bot] merged 12 commits into
NVIDIA:branch-22.08from
ttnghia:fix_min_max

Conversation

@ttnghia

@ttnghia ttnghia commented Jul 26, 2022

Copy link
Copy Markdown
Contributor

This fixes a bug of device operators min and max in generating the identity value for floating-point numbers. In particular:

  • min::identity() should return cuda::std::numeric_limits<T>::infinity() instead of cuda::std::numeric_limits<T>::max(), and
  • max::identity() should return -cuda::std::numeric_limits<T>::infinity() instead of cuda::std::numeric_limits<T>::lowest().

Closes #11352.

@ttnghia ttnghia added bug Something isn't working 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. Spark Functionality that helps Spark RAPIDS non-breaking Non-breaking change labels Jul 26, 2022
@ttnghia ttnghia self-assigned this Jul 26, 2022
@ttnghia
ttnghia requested a review from a team as a code owner July 26, 2022 18:16

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

The changes look good but we need to add tests for this.

@ttnghia
ttnghia marked this pull request as draft July 26, 2022 21:37
@ttnghia
ttnghia marked this pull request as ready for review July 26, 2022 23:27

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

Question about mixing std::numeric_limits<HostType> and cuda::std::numeric_limits<T>, otherwise LGTM.

Comment thread cpp/tests/reductions/scan_tests.cpp Outdated
Comment thread cpp/tests/reductions/scan_tests.cpp Outdated
Comment thread cpp/tests/reductions/scan_tests.cpp Outdated
Comment thread cpp/tests/groupby/min_tests.cpp Outdated
@ttnghia

ttnghia commented Jul 28, 2022

Copy link
Copy Markdown
Contributor Author

@gpucibot merge

@codecov

codecov Bot commented Jul 28, 2022

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.08@ec79ce1). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-22.08   #11357   +/-   ##
===============================================
  Coverage                ?   86.43%           
===============================================
  Files                   ?      143           
  Lines                   ?    22777           
  Branches                ?        0           
===============================================
  Hits                    ?    19687           
  Misses                  ?     3090           
  Partials                ?        0           

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 ec79ce1...d5bdfff. Read the comment docs.

@rapids-bot
rapids-bot Bot merged commit cfc17c6 into NVIDIA:branch-22.08 Jul 28, 2022
@ttnghia
ttnghia deleted the fix_min_max branch July 28, 2022 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3 - Ready for Review Ready for review by team bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Spark Functionality that helps Spark RAPIDS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Min and Max aggregations involving infinity produce incorrect results

5 participants