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.

Sign up for GitHub

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

Jump to bottom

[REVIEW] find_and_replace function #350

Merged
merged 16 commits into from
Dec 17, 2018
Merged

[REVIEW] find_and_replace function #350

merged 16 commits into from
Dec 17, 2018

Conversation

wmalpica
Copy link

@wmalpica wmalpica commented Nov 8, 2018

Replace function moved from old libgdf repo. It was on PR# 106

@kkraus14
Copy link
Collaborator

kkraus14 commented Nov 8, 2018

@mt-jones @dantegd I assume hold off on merging this until after the refactor is pushed and completed?

@kkraus14 kkraus14 added the 3 - Ready for Review Ready for review by team label Nov 8, 2018
jrhemstad
jrhemstad requested changes Nov 9, 2018
View reviewed changes
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.

May want to hold off on merging this until I've got my new type_dispatcher merged in, because this is a perfect candidate for using it. See here: https://github.com/rapidsai/cudf/blob/fb8163df529574e3c75603a5e3c14d3164dc3245/libgdf/src/gdf_type_dispatcher.cuh

libgdf/src/bench/CMakeLists.txt Outdated Show resolved Hide resolved
libgdf/src/bench/replace/replace-benchmark.cu Outdated Show resolved Hide resolved
libgdf/src/bench/replace/CMakeLists.txt Outdated Show resolved Hide resolved
harrism
harrism requested changes Nov 13, 2018
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.

One performance issue with the kernel to be checked, and some other requests.

libgdf/include/gdf/cffi/functions.h Outdated Show resolved Hide resolved
libgdf/src/bench/CMakeLists.txt Outdated Show resolved Hide resolved
libgdf/src/replace.cu Outdated Show resolved Hide resolved
libgdf/src/replace.cu Outdated Show resolved Hide resolved
libgdf/src/tests/replace/utils.h Outdated Show resolved Hide resolved
@harrism harrism added 0 - Waiting on Author Waiting for author to respond to review and removed 3 - Ready for Review Ready for review by team labels Nov 13, 2018
@randerzander randerzander mentioned this pull request Nov 13, 2018
Merged
@harrism harrism added the libcudf Affects libcudf (C++/CUDA) code. label Nov 25, 2018
@harrism
Copy link
Member

harrism commented Nov 28, 2018

@williamBlazing the type_dispatcher PR (#379) from @jrhemstad is now merged. Could you rework this PR to use it? It should simplify your code. :)

@jeanp413
Refactor replace unit tests.
7fdf5dc 
Updated doxigen documentation.
Fixed compilation erros.
harrism
harrism approved these changes Dec 5, 2018
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.

This is looking really good now. The type_dispatcher really simplifies things.

cpp/src/replace/replace.cu Show resolved Hide resolved
cpp/src/replace/replace.cu Show resolved Hide resolved
@harrism
Copy link
Member

harrism commented Dec 5, 2018

@jrhemstad can you review the new changes? @williamBlazing once you resolve conflicts and fix CI errors it looks good to go. Will also need to update the CHANGELOG (new check).

jrhemstad
jrhemstad requested changes Dec 5, 2018
View reviewed changes
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.

Thanks @williamBlazing !

cpp/include/cudf/functions.h Outdated Show resolved Hide resolved
cpp/include/cudf/functions.h Show resolved Hide resolved
cpp/src/replace/replace.cu Outdated Show resolved Hide resolved
cpp/src/replace/replace.cu Outdated Show resolved Hide resolved
cpp/src/replace/replace.cu Outdated Show resolved Hide resolved
@harrism harrism changed the base branch from master to branch-0.5 December 10, 2018 04:25
harrism
harrism requested changes Dec 12, 2018
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.

Nearly there. For the most part this code looks great. Thanks guys! One change from thrust::device_vector to rmm::device_vector needed, and suggesting some cosmetic doc changes.

CHANGELOG.md Show resolved Hide resolved
cpp/include/cudf/functions.h Outdated Show resolved Hide resolved
cpp/include/cudf/functions.h Outdated Show resolved Hide resolved
cpp/src/replace/replace.cu Outdated Show resolved Hide resolved
cpp/src/replace/replace.cu Outdated Show resolved Hide resolved
cpp/src/replace/replace.cu Outdated Show resolved Hide resolved
cpp/src/replace/replace.cu Outdated Show resolved Hide resolved
cpp/src/replace/replace.cu Show resolved Hide resolved
cpp/src/replace/replace.cu Outdated Show resolved Hide resolved
cpp/src/replace/replace.cu Outdated Show resolved Hide resolved
@harrism harrism changed the title [REVIEW] Replace function [REVIEW] find_and_replace function Dec 13, 2018
@harrism
Copy link
Member

harrism commented Dec 16, 2018

I just realized that this implements the C++ side but the Python side is still to do. Do you guys have plans for this @williamBlazing ?

@harrism harrism added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 0 - Waiting on Author Waiting for author to respond to review labels Dec 16, 2018
@harrism
Copy link
Member

harrism commented Dec 16, 2018

rerun tests. Grrrrr.

@shwina shwina mentioned this pull request Dec 17, 2018
Merged
@kkraus14
Copy link
Collaborator

run tests

@kkraus14
Copy link
Collaborator

@harrism Python bindings are a WIP in #539, I'll defer to you to merge this PR 😄.

@harrism
Copy link
Member

harrism commented Dec 17, 2018 •
edited
Loading

I was waiting on CI to merge. This one is fine for anyone to merge (I added the "ready to merge" tag) once CI passes.

@kkraus14 kkraus14 merged commit 2efc6c3 into rapidsai:branch-0.5 Dec 17, 2018
@harrism
Copy link
Member

harrism commented Dec 17, 2018

@kkraus14 are we ignoring CI failures now?

@kkraus14
Copy link
Collaborator

@harrism as long as the required ones are passing we can ignore the HANGS for now. If a test explicitly fails with actual output that shouldn't be ignored.

@jrhemstad jrhemstad mentioned this pull request Dec 20, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrhemstad jrhemstad jrhemstad approved these changes

@harrism harrism harrism approved these changes

Assignees

@wmalpica wmalpica

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wmalpica @kkraus14 @harrism @jrhemstad @jeanp413