Skip to content

Add conversions between column_view and device_span<T const>.#10302

Merged
rapids-bot[bot] merged 18 commits intorapidsai:branch-22.04from
bdice:column_view-to-device_span-implicit-conversion
Feb 17, 2022
Merged

Add conversions between column_view and device_span<T const>.#10302
rapids-bot[bot] merged 18 commits intorapidsai:branch-22.04from
bdice:column_view-to-device_span-implicit-conversion

Conversation

@bdice
Copy link
Contributor

@bdice bdice commented Feb 15, 2022 •
edited
Loading

This PR adds an implicit conversion operator from column_view to device_span<T const>. The immediate purpose of this PR is to make it possible to use the API segmented_reduce(column_view data, device_span<size_type> offsets, ...) in PR #9621.

This PR also resolves #9656 by adding a column_view constructor from device_span<T const>.

More broadly, this PR should make it easier to refactor instances where column.data() is used with counting iterators to build transform iterators, or other patterns that require a length (e.g. vector factories to copy to host).

@bdice bdice self-assigned this Feb 15, 2022
@bdice bdice added improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. labels Feb 15, 2022
@bdice bdice mentioned this pull request Feb 15, 2022
Merged
@bdice bdice added non-breaking Non-breaking change 2 - In Progress Currently a work in progress labels Feb 15, 2022
@codecov
Copy link

codecov bot commented Feb 16, 2022 •
edited
Loading

Codecov Report

Merging #10302 (82b44b7) into branch-22.04 (a7d88cd) will increase coverage by 0.21%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-22.04   #10302      +/-   ##
================================================
+ Coverage         10.42%   10.63%   +0.21%     
================================================
  Files               119      122       +3     
  Lines             20603    20940     +337     
================================================
+ Hits               2148     2228      +80     
- Misses            18455    18712     +257
Impacted Files Coverage Δ
python/cudf/cudf/_fuzz_testing/fuzzer.py 0.00% <ø> (ø)
python/cudf/cudf/_fuzz_testing/io.py 0.00% <ø> (ø)
python/cudf/cudf/_fuzz_testing/main.py 0.00% <ø> (ø)
python/cudf/cudf/_version.py 0.00% <ø> (ø)
python/cudf/cudf/comm/gpuarrow.py 0.00% <ø> (ø)
python/cudf/cudf/core/_base_index.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/categorical.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/column.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/datetime.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/methods.py 0.00% <ø> (ø)
... and 59 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 f263820...82b44b7. Read the comment docs.

@github-actions github-actions bot added the CMake CMake build issue label Feb 16, 2022
jrhemstad
jrhemstad reviewed Feb 16, 2022
View reviewed changes
@bdice bdice changed the title Add implicit conversion operator from column_view to device_span<T>. Add conversions between column_view and device_span<T>. Feb 16, 2022
jrhemstad
jrhemstad reviewed Feb 16, 2022
View reviewed changes
jrhemstad
jrhemstad reviewed Feb 16, 2022
View reviewed changes
jrhemstad
jrhemstad reviewed Feb 16, 2022
View reviewed changes
@bdice bdice marked this pull request as ready for review February 17, 2022 03:06
@bdice bdice requested a review from a team as a code owner February 17, 2022 03:06
@bdice bdice requested review from harrism and vyasr February 17, 2022 03:06
@bdice bdice requested a review from jrhemstad February 17, 2022 03:06
@bdice bdice changed the title Add conversions between column_view and device_span<T>. Add conversions between column_view and device_span<T const>. Feb 17, 2022
davidwendt
davidwendt reviewed Feb 17, 2022
View reviewed changes
@bdice @davidwendt
Update cpp/include/cudf/column/column_view.hpp
8c4783a 
Co-authored-by: David Wendt <45795991+davidwendt@users.noreply.github.com>
harrism
harrism approved these changes Feb 17, 2022
View reviewed changes
Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Nice addition.

davidwendt
davidwendt reviewed Feb 17, 2022
View reviewed changes
davidwendt
davidwendt reviewed Feb 17, 2022
View reviewed changes
davidwendt
davidwendt requested changes Feb 17, 2022
View reviewed changes
Copy link
Contributor

@davidwendt davidwendt left a comment

Choose a reason for hiding this comment

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

Could you add some gtests that exercise the CUDF_EXPECTS checks?

davidwendt
davidwendt reviewed Feb 17, 2022
View reviewed changes
@bdice
Copy link
Contributor Author

bdice commented Feb 17, 2022

Could you add some gtests that exercise the CUDF_EXPECTS checks?

Yes, I think that's a good idea! I'll work on that.

bdice and others added 2 commits February 17, 2022 10:44
@bdice @davidwendt
Apply suggestions from code review
62ee87b 
Co-authored-by: David Wendt <45795991+davidwendt@users.noreply.github.com>
@bdice
Add tests for column_view failing to convert to device_span.
82b44b7
@bdice bdice requested a review from davidwendt February 17, 2022 18:40
@bdice
Copy link
Contributor Author

bdice commented Feb 17, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit d48dd6f into rapidsai:branch-22.04 Feb 17, 2022
@davidwendt davidwendt mentioned this pull request Feb 18, 2022
Merged
rapids-bot bot pushed a commit that referenced this pull request Feb 19, 2022
@davidwendt
Fix debug compile error in device_span to column_view conversion (#10331
527d4ee 
)

Fixes a compile error in `column_view(device_span)` constructor when building libcudf in Debug.

```
Building CXX object tests/CMakeFi...TEST.dir/column/column_view_device_span_test.cpp.o
FAILED: tests/CMakeFiles/COLUMN_TEST.dir/column/column_view_device_span_test.cpp.o 
/usr/local/bin/g++ -DCUDF_VERSION=22.04.00 -DGTEST_LINKED_AS_SHARED_LIBRARY=1 -DJITIFY_USE_CACHE -DSPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_INFO -DTHRUST_DEVICE_SYSTEM=THRUST_DEVICE_SYSTEM_CUDA -DTHRUST_HOST_SYSTEM=THRUST_HOST_SYSTEM_CPP -I/cudf/cpp -I/cudf/cpp/src -I/cudf/cpp/build/_deps/dlpack-src/include -I/cudf/cpp/build/_deps/jitify-src -I/cudf/cpp/include -I/cudf/cpp/build/include -I/conda/envs/rapids/include/rapids/libcudacxx -I/cudf/cpp/build/_deps/thrust-src -I/cudf/cpp/build/_deps/thrust-src/dependencies/cub -isystem /conda/envs/rapids/include -isystem /usr/local/cuda/include -fdiagnostics-color=always -g -fPIE -Wall -Werror -Wno-unknown-pragmas -Wno-error=deprecated-declarations -Wno-deprecated-declarations -pthread -std=gnu++17 -MD -MT tests/CMakeFiles/COLUMN_TEST.dir/column/column_view_device_span_test.cpp.o -MF tests/CMakeFiles/COLUMN_TEST.dir/column/column_view_device_span_test.cpp.o.d -o tests/CMakeFiles/COLUMN_TEST.dir/column/column_view_device_span_test.cpp.o -c /cudf/cpp/tests/column/column_view_device_span_test.cpp
In file included from /cudf/cpp/include/cudf/column/column_view.hpp:19,
                 from /cudf/cpp/tests/column/column_view_device_span_test.cpp:17:
/cudf/cpp/include/cudf/column/column_view.hpp: In instantiation of ‘cudf::column_view::column_view(cudf::device_span<const T>) [with T = float; std::enable_if_t<(is_numeric<T>() || is_chrono<T>())>* <anonymous> = 0]’:
/cudf/cpp/tests/column/column_view_device_span_test.cpp:55:21:   required from ‘void ColumnViewDeviceSpanTests_conversion_round_trip_Test<gtest_TypeParam_>::TestBody() [with gtest_TypeParam_ = float]’
/cudf/cpp/tests/column/column_view_device_span_test.cpp:48:1:   required from here
/cudf/cpp/include/cudf/column/column_view.hpp:396:30: error: comparison of integer expressions of different signedness: ‘cudf::detail::span_base<const float, 18446744073709551615, cudf::device_span<const float> >::size_type’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare]
  396 |     CUDF_EXPECTS(data.size() < std::numeric_limits<cudf::size_type>::max(),

```

The above also includes the command line compile that creates the error.
The solution implemented in this PR casts the `size_type` `max()` value to `std::size_t` before comparing it to the span's `size()` value.

The change implemented in #10302 introduced this conversion.

Authors:
  - David Wendt (https://github.com/davidwendt)

Approvers:
  - Vukasin Milovanovic (https://github.com/vuule)
  - Nghia Truong (https://github.com/ttnghia)

URL: #10331
This was referenced Jun 15, 2022
Merged
Closed
@bdice bdice mentioned this pull request Jul 26, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@harrism harrism harrism approved these changes

@jrhemstad jrhemstad jrhemstad approved these changes

@davidwendt davidwendt davidwendt approved these changes

@vyasr vyasr Awaiting requested review from vyasr vyasr is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

Assignees

@bdice bdice

Labels

2 - In Progress Currently a work in progress CMake CMake build issue improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEA] Construct column_view from rmm::device_uvector and device_span

4 participants

@bdice @harrism @jrhemstad @davidwendt