[REVIEW] Port unary to libcudf++ by codereport · Pull Request #3214 · rapidsai/cudf

codereport · 2019-10-25T06:20:13Z

[ISSUE] 2950

Port math_ops.cu
Port unary_ops.cuh

null_ops was done in: 3182
cast_ops was done in: 3397

codecov · 2019-10-25T07:53:26Z

Codecov Report

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

@@             Coverage Diff              @@
##           branch-0.11    #3214   +/-   ##
============================================
  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 1f001ae...d931b0a. Read the comment docs.

[ISSUE] 2950 Port null_ops.cu

harrism · 2019-10-25T16:18:57Z

@codereport please avoid force pushes (e.g. use merge rather than rebase) once you have an open PR associated with a feature branch. Github doesn't handle force pushes well. It's most important once people have reviewed the code, because force pushing disassociates comments from the current state of the code.

codereport · 2019-10-26T09:13:59Z

please avoid force pushes

Will avoid these in the future! Thanks!

jrhemstad

It looks like there was some miscommunication and porting these APIs was already done here: #3182

harrism · 2019-10-26T21:45:24Z

@jrhemstad Unary ops encompasses a lot more than just null_ops! But yes, null_ops is a new feature added by @rgsl888prabhu in #2962 and immediately ported to libcudf in #3182. @codereport I don't think you need to touch null_ops, just everything else in unary.hpp

codereport · 2019-10-27T01:02:36Z

@jrhemstad @harrism : @rgsl888prabhu did mention he had code changes for is_null and is_not_null but I though we was just letting me know about the pre-ported changes he merged. Probably could have been avoided if I was notified of the PR numbers (if they aren't already merged that is).

I will focus on the 3 other files.

harrism · 2019-10-27T21:57:32Z

null_ops did not exist when I originally created #2950.

codereport · 2019-10-28T10:12:24Z

@harrism Sorry Mark! I wasn't clear - I meant that if Ram had have let me know what the PR #'s were I could have checked. I will just make sure in the future to ask for them :)

rgsl888prabhu · 2019-10-28T14:27:24Z

I apologize for any inconvenience caused by me.

Remove port of null_ops.cu It is duplicate work that has been done in 3182

harrism · 2019-10-29T01:24:35Z

GitHub has a pretty good PR search function. :)

raydouglass

Nothing actually for ops to review. Maybe from a merge or force push, but approving to prevent blocking the PR.

[ISSUE] 2950 Initial port math_ops.cu & unary_ops.cuh

jrhemstad · 2019-10-29T12:13:24Z

@codereport just as a general note, I think the old implementation could be vastly improved by replacing the custom kernel with a thrust::transform.

codereport · 2019-10-29T17:13:45Z

thrust::transform

Thanks @jrhemstad! I will definitely do that

Co-Authored-By: Mark Harris <mharris@nvidia.com>

harrism · 2019-11-24T23:09:08Z

@codereport is this ready for review? If so, please mark it so and we can do another review pass.

codereport · 2019-11-25T17:52:46Z

@harrism I am just in the process of adding a couple extra tests, will mark as [REVIEW] once I have added these.

Change .begin/end to .cbegin/cend

harrism

Nearly there. Some test suggestions.

harrism · 2019-11-26T00:41:40Z

+    }
+
+    template <typename T,
+              typename std::enable_if_t<std::is_same<T, cudf::experimental::bool8>::value>* = nullptr>


How about factoring out the casting so that all operators can just call a single function rather than maintaining two copies of every operator? Something that calls the actual op with casts only for types that need it, like this. Could be part of a base class that all device_ops inherit.

template<typename T, typename Op, std::enable_if_t<!std::is_same<T, cudf::experimental::bool8>::value>* = nullptr> __device__ T normalized_unary_op()(Op op, T data) { return Op(data); } template<typename T, std::enable_if_t<std::is_same<T, cudf::experimental::bool8>::value>* = nullptr> __device__ T normalized_unary_op()(T data) { return static_cast<T>(Op(static_cast<float>(data)); }

- update CHANGELOG msg - refactor math_ops SFINAE to reduce code bloat - add some "empty" unit tests - increase test/vector size

jrhemstad · 2019-11-26T16:08:38Z

+    }
+
+    template <typename T,
+              typename std::enable_if_t<std::is_same<T, cudf::experimental::bool8>::value>* = nullptr>


This needs to be resolved in the bool8 type and not externally in functions like this. I'm not sure what the right combinations of implicit/explicit conversion and operators is necessary, but it's not sustainable to have to do things like what was done here in providing two versions of every operator.

jrhemstad · 2019-11-26T16:22:38Z

+
+    if (input.size() == 0) return output;
+
+    auto output_view = output->mutable_view();


This pattern of passing in the output as a mutable_view prevents any future support of string or other non fixed-width type columns. Granted, most of these unary ops don't make sense on strings. However, I believe these should be modified to return their output rather than pass via a mutable view.

dispatcher / launcher is updated to return std::unique_ptr<cudf::column_view>

- additional empty column test - remove stream from public (non-detail) API

- add tests to cover failure cases

codereport requested review from a team as code owners October 25, 2019 06:20

codereport requested a review from a team October 25, 2019 06:20

codereport force-pushed the fea-port-unary branch from 293c698 to 8826ad8 Compare October 25, 2019 06:22

[WIP] Port unary to libcudf++

14a3f63

[ISSUE] 2950 Port null_ops.cu

codereport force-pushed the fea-port-unary branch from 8826ad8 to 14a3f63 Compare October 25, 2019 15:59

jrhemstad requested changes Oct 26, 2019

View reviewed changes

Merge branch 'branch-0.11' into fea-port-unary

e919b4b

codereport added 2 commits October 28, 2019 13:52

[ISSUE] 2950

a83a1c7

Remove port of null_ops.cu It is duplicate work that has been done in 3182

Merge branch 'branch-0.11' into fea-port-unary

08d2154

raydouglass approved these changes Oct 29, 2019

View reviewed changes

harrism added 2 - In Progress Currently a work in progress libcudf++ labels Oct 29, 2019

codereport added 2 commits October 29, 2019 01:01

[WIP] Port unary to libcudf++

4e9c944

[ISSUE] 2950 Initial port math_ops.cu & unary_ops.cuh

Merge branch 'branch-0.11' into fea-port-unary

9f39da7

harrism requested changes Oct 30, 2019

View reviewed changes

Comment thread cpp/src/unary/math_ops.cu Outdated

Comment thread cpp/src/unary/math_ops.cu Outdated

Comment thread cpp/src/unary/unary_ops.cuh Outdated

Comment thread cpp/src/unary/unary_ops.cuh Outdated

Comment thread cpp/src/unary/unary_ops.cuh Outdated

codereport and others added 3 commits October 30, 2019 14:32

Merge branch 'branch-0.11' into fea-port-unary

ac250c5

Update cpp/src/unary/math_ops.cu

d874e7c

Co-Authored-By: Mark Harris <mharris@nvidia.com>

Update cpp/src/unary/unary_ops.cuh

587c4f9

Co-Authored-By: Mark Harris <mharris@nvidia.com>

Renaming functions / structs from CR

194e2e6

Initial unary port tests

dc514c1

jrhemstad reviewed Nov 25, 2019

View reviewed changes

Comment thread cpp/tests/unary/legacy/unary_ops_test.cu

codereport added 2 commits November 25, 2019 15:10

Remove tabs from unary_ops_tests

2f6e524

Change .begin/end to .cbegin/cend

Extra unary ops tests

317c199

trxcllnt reviewed Nov 25, 2019

View reviewed changes

Comment thread cpp/src/unary/unary_ops.cuh Outdated

codereport added 2 commits November 25, 2019 15:28

Extra unary ops tests

789610d

Merge remote-tracking branch 'origin/fea-port-unary' into fea-port-unary

067dd12

codereport changed the title ~~[WIP] Port unary to libcudf++~~ [REVIEW] Port unary to libcudf++ Nov 25, 2019

codereport added the 3 - Ready for Review Ready for review by team label Nov 25, 2019

Small fix and clean up of handle_checks_and_validity

fe7ee5e

codereport requested a review from trxcllnt November 25, 2019 21:28

Merge branch 'branch-0.11' into fea-port-unary

a1d3c07

codereport requested review from harrism and jrhemstad November 25, 2019 21:57

harrism requested changes Nov 26, 2019

View reviewed changes

Responding to CR comments:

feba50b

- update CHANGELOG msg - refactor math_ops SFINAE to reduce code bloat - add some "empty" unit tests - increase test/vector size

jrhemstad requested changes Nov 26, 2019

View reviewed changes

codereport added 3 commits November 26, 2019 14:39

Responding to CR comments:

088e3ad

- additional empty column test - remove stream from public (non-detail) API

Refactor of dispatcher/launcher to return column_view

480ac5b

Responding to CR comments:

870d7a1

- add tests to cover failure cases

codereport requested review from harrism and jrhemstad November 27, 2019 01:55

harrism approved these changes Nov 27, 2019

View reviewed changes

Merge branch 'branch-0.11' into fea-port-unary

d931b0a

jrhemstad approved these changes Nov 27, 2019

View reviewed changes

jrhemstad merged commit d206d35 into rapidsai:branch-0.11 Nov 27, 2019

harrism mentioned this pull request Nov 29, 2019

[FEA] Convert unary.hpp to cudf::column types #2950

Closed


		if (input.size() == 0) return output;

		auto output_view = output->mutable_view();

Conversation

codereport commented Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

harrism commented Oct 25, 2019

Uh oh!

codereport commented Oct 26, 2019

Uh oh!

jrhemstad left a comment

Choose a reason for hiding this comment

Uh oh!

harrism commented Oct 26, 2019

Uh oh!

codereport commented Oct 27, 2019

Uh oh!

harrism commented Oct 27, 2019

Uh oh!

codereport commented Oct 28, 2019

Uh oh!

rgsl888prabhu commented Oct 28, 2019

Uh oh!

harrism commented Oct 29, 2019

Uh oh!

raydouglass left a comment

Choose a reason for hiding this comment

Uh oh!

jrhemstad commented Oct 29, 2019

Uh oh!

codereport commented Oct 29, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

harrism commented Nov 24, 2019

Uh oh!

codereport commented Nov 25, 2019

Uh oh!

Uh oh!

Uh oh!

harrism left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

harrism Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrhemstad Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

jrhemstad Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

codereport Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codereport commented Oct 25, 2019 •

edited

Loading

codecov bot commented Oct 25, 2019 •

edited

Loading