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

Cleaning up for loops with make_(counting_)transform_iterator #6546

Merged
merged 15 commits into from
Feb 6, 2021

Conversation

codereport
Copy link
Contributor

@codereport codereport commented Oct 16, 2020

Doing some cleanup - using make_counting_transform_iterator and make_transform_iterator to clean up for loops and other code.

Making an argument that make_counting_transform_iterator should be moved out of cudf::test::.

Also, will clean up the f/begin and op naming conventions.

Depends on: #7306

@codereport codereport added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. labels Oct 16, 2020
@codereport codereport requested a review from a team as a code owner October 16, 2020 01:49
@codereport codereport self-assigned this Oct 16, 2020
@GPUtester
Copy link
Collaborator

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

Copy link
Contributor

@jrhemstad jrhemstad left a comment

Choose a reason for hiding this comment

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

Some of these updates could be even cleaner by just using the child iterators.

cpp/src/column/column_view.cpp Show resolved Hide resolved
cpp/src/copying/copy.cpp Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Oct 16, 2020

Codecov Report

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

Impacted file tree graph

@@              Coverage Diff               @@
##             branch-0.19    #6546   +/-   ##
==============================================
  Coverage               ?   82.20%           
==============================================
  Files                  ?      100           
  Lines                  ?    16966           
  Branches               ?        0           
==============================================
  Hits                   ?    13947           
  Misses                 ?     3019           
  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 8215b5b...adda037. Read the comment docs.

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 improvements, and even cleaner with Jake's observation about child iterators.

Please move make_counting_transform_iterator into a libcudf header so we don't have to depend on libcudf_test and so it's available to users of libcudf that are not users of libcudf_test.

cpp/src/column/column_view.cpp Outdated Show resolved Hide resolved
cpp/src/column/column_view.cpp Show resolved Hide resolved
cpp/src/copying/copy.cpp Outdated Show resolved Hide resolved
cpp/src/copying/slice.cpp Outdated Show resolved Hide resolved
cpp/src/copying/slice.cpp Outdated Show resolved Hide resolved
cpp/src/copying/slice.cpp Outdated Show resolved Hide resolved
cpp/src/interop/to_arrow.cpp Outdated Show resolved Hide resolved
@devavret devavret removed their request for review October 20, 2020 03:34
@codereport
Copy link
Contributor Author

Do the following:

// cpp/src/structs/structs_column_view.cu
  std::vector<column_view> children;
  children.reserve(child(index).num_children());
  for (size_type i = 0; i < child(index).num_children(); i++) {
    children.push_back(child(index).child(i));
  }

@harrism
Copy link
Member

harrism commented Dec 2, 2020

@codereport should this slip to 0.18?

@codereport codereport changed the title [WIP] Cleaning up for loops with make_counting_transform_iterator Cleaning up for loops with make_counting_transform_iterator Dec 11, 2020
@codereport codereport marked this pull request as draft December 11, 2020 00:24
@jrhemstad
Copy link
Contributor

@codereport do you still plan to complete this? Moving to 0.19.

@codereport codereport changed the base branch from branch-0.17 to branch-0.19 February 3, 2021 18:01
@codereport codereport added code quality non-breaking Non-breaking change labels Feb 3, 2021
@codereport codereport added 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond and removed 2 - In Progress Currently a work in progress labels Feb 5, 2021
@codereport codereport changed the title Cleaning up for loops with make_counting_transform_iterator Cleaning up for loops with make_(counting_)transform_iterator Feb 5, 2021
@codereport codereport marked this pull request as ready for review February 5, 2021 13:40
cpp/src/copying/slice.cpp Outdated Show resolved Hide resolved
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.

LGTM. Some of the files need a copyright year update.

@codereport codereport requested a review from jrhemstad February 5, 2021 15:48
@codereport codereport added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond labels Feb 6, 2021
@codereport
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 621de88 into rapidsai:branch-0.19 Feb 6, 2021
rapids-bot bot pushed a commit that referenced this pull request Feb 9, 2021
Came across some "raw" uses of `thrust::make_transform_iterator` + `thrust::make_counting_iterator` where now `cudf::detail::make_counting_transform_iterator` can be used (because of #6546).

Authors:
  - Conor Hoekstra (@codereport)

Approvers:
  - Devavret Makkar (@devavret)
  - David (@davidwendt)
  - Mark Harris (@harrism)

URL: #7338
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants