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.

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

Add fixed point to AllTypes in libcudf unit tests #9472

Merged

Conversation

karthikeyann
Copy link
Contributor

Fixes part of #9468

  • ITERATOR_TEST
    • excludes fixed point because it's unused anywhere now.
    • but this needs fix to fixed_point_scalar and fixed_point_scalar_device_view.
  • REDUCTION_TEST
    • added fixed point support for minmax.
    • Also needs fix from fixed_point_scalar::value() (to avoid T{} cast)

@karthikeyann karthikeyann added bug Something isn't working 3 - Ready for Review Ready for review by team tests Unit testing for project libcudf Affects libcudf (C++/CUDA) code. 4 - Needs Review Waiting for reviewer to review or respond non-breaking Non-breaking change labels Oct 19, 2021
@karthikeyann karthikeyann requested a review from a team as a code owner October 19, 2021 15:56
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@karthikeyann
Copy link
Contributor Author

karthikeyann commented Oct 19, 2021

@codereport do fixed_point_scalar and fixed_point_scalar_device_view methods need some modification?
scalar.hpp

fixed_point_scalar<T> fixed_width_scalar<T>
rep_type value() T value()
rep_type* data() T* data()
T fixed_point_value()

scalar_device_view.cuh

fixed_point_scalar_device_view<T> fixed_width_scalar_device_view<T>
T& value()
rep_type const& rep()
void set_value(rep_type value) void set_value(T value)
T* data()

@karthikeyann karthikeyann added the 5 - DO NOT MERGE Hold off on merging; see PR for details label Oct 19, 2021
@karthikeyann karthikeyann removed the 5 - DO NOT MERGE Hold off on merging; see PR for details label Oct 19, 2021
@codecov
Copy link

codecov bot commented Oct 19, 2021

Codecov Report

Merging #9472 (83466f0) into branch-21.12 (ab4bfaa) will decrease coverage by 0.12%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-21.12    #9472      +/-   ##
================================================
- Coverage         10.79%   10.66%   -0.13%     
================================================
  Files               116      117       +1     
  Lines             18869    19755     +886     
================================================
+ Hits               2036     2106      +70     
- Misses            16833    17649     +816     
Impacted Files Coverage Δ
python/cudf/cudf/io/csv.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/hdf.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/orc.py 0.00% <0.00%> (ø)
python/cudf/cudf/__init__.py 0.00% <0.00%> (ø)
python/cudf/cudf/_version.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/abc.py 0.00% <0.00%> (ø)
python/cudf/cudf/api/types.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/dlpack.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/frame.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/index.py 0.00% <0.00%> (ø)
... and 61 more

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 5e2aaf9...83466f0. Read the comment docs.

Copy link
Contributor

@codereport codereport left a comment

Choose a reason for hiding this comment

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

👍

@codereport
Copy link
Contributor

@codereport do fixed_point_scalar and fixed_point_scalar_device_view methods need some modification? scalar.hpp

fixed_point_scalar<T> fixed_width_scalar<T>
rep_type value() T value()
rep_type* data() T* data()
T fixed_point_value()
scalar_device_view.cuh

fixed_point_scalar_device_view<T> fixed_width_scalar_device_view<T>
T& value()
rep_type const& rep()
void set_value(rep_type value) void set_value(T value)
T* data()

I would say don't change it. I definitely recall that I explicitly added the fixed_point_value() on the non device type, but I don't think we want't that on the device one. If there is a need down that line for some of the "missing" methods we can revisit it then.

@karthikeyann
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit bb98505 into rapidsai:branch-21.12 Oct 20, 2021
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 4 - Needs Review Waiting for reviewer to review or respond bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change tests Unit testing for project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants