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

[REVIEW] Port scatter to tables to libcudf++ #3448

Merged
merged 18 commits into from
Nov 28, 2019

Conversation

trevorsm7
Copy link
Contributor

Left out of PR #3354
Needed for PR #3144

@trevorsm7 trevorsm7 added 3 - Ready for Review Ready for review by team libcudf++ labels Nov 23, 2019
@trevorsm7 trevorsm7 requested review from a team as code owners November 23, 2019 04:47
@trevorsm7 trevorsm7 requested a review from a team November 23, 2019 04:47
@trevorsm7 trevorsm7 self-assigned this Nov 23, 2019
@trevorsm7 trevorsm7 mentioned this pull request Nov 23, 2019
7 tasks
@codecov
Copy link

codecov bot commented Nov 23, 2019

Codecov Report

Merging #3448 into branch-0.11 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           branch-0.11    #3448   +/-   ##
============================================
  Coverage        87.35%   87.35%           
============================================
  Files               49       49           
  Lines             9329     9329           
============================================
  Hits              8149     8149           
  Misses            1180     1180

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 265c500...55fdd75. 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.

Looks really good. Just a few comments.

cpp/include/cudf/copying.hpp Outdated Show resolved Hide resolved
cpp/include/cudf/copying.hpp Outdated Show resolved Hide resolved
cudaStream_t stream = 0);

/**
* @brief Scatters the rows of a table to `n` tables according to a partition map
Copy link
Member

Choose a reason for hiding this comment

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

Not necessarily for this PR, but I think we should find a way to not have to maintain two copies of the docs which can diverge (detail and non-detail APIs).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cpp/tests/copying/scatter_to_tables_tests.cu Outdated Show resolved Hide resolved
cpp/tests/utilities/column_utilities.cu Outdated Show resolved Hide resolved
cpp/src/copying/scatter.cu Show resolved Hide resolved
cpp/src/copying/scatter.cu Show resolved Hide resolved
cpp/tests/copying/scatter_to_tables_tests.cu Outdated Show resolved Hide resolved
@trevorsm7 trevorsm7 requested a review from harrism November 27, 2019 21:17
@trevorsm7 trevorsm7 merged commit 409f560 into rapidsai:branch-0.11 Nov 28, 2019
@trevorsm7 trevorsm7 deleted the fea-port-scatter-to-tables branch December 3, 2019 02:04
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants