[REVIEW] Add `cudf::encode` #5572

shwina · 2020-06-24T22:51:13Z

Essentially moves functionality out of encode.cu into a public API and adds tests for it. See #5498.

GPUtester · 2020-06-24T22:51:48Z

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

cpp/src/transform/codify.cpp

harrism

Hang on, isn't this what a dictionary column is for? Isn't this just dictionary encoding with sequential integer keys? CC @davidwendt

cpp/include/cudf/transform.hpp

shwina · 2020-06-25T11:10:04Z

Yes - this PR is just exposing functionality already written for dictionary columns. I guess we could just cast the input column to a dictionary column and then grab the indices from there, but it feels like an implementation detail that the indices will always be 0 to N-1.

Co-authored-by: Mark Harris <[email protected]>

shwina · 2020-06-25T11:16:03Z

Should have xref'd issue: #5498

…dd-codify

cpp/src/transform/encode.cpp

cpp/tests/encode/encode_tests.cpp

davidwendt · 2020-07-21T20:28:22Z

cpp/tests/encode/encode_tests.cpp

+#include <tests/utilities/base_fixture.hpp>
+#include <tests/utilities/column_utilities.hpp>
+#include <tests/utilities/column_wrapper.hpp>
+#include <tests/utilities/table_utilities.hpp>


Is tests/utilities/table_utilities.hpp needed?

cpp/tests/CMakeLists.txt

Co-authored-by: David <[email protected]>

codecov · 2020-07-21T22:32:42Z

Codecov Report

Merging #5572 into branch-0.15 will increase coverage by 0.00%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           branch-0.15    #5572   +/-   ##
============================================
  Coverage        86.38%   86.38%           
============================================
  Files               76       76           
  Lines            13033    13036    +3     
============================================
+ Hits             11258    11261    +3     
  Misses            1775     1775

Impacted Files	Coverage Δ
python/cudf/cudf/core/index.py	`92.24% <0.00%> (-0.03%)`	⬇️
python/cudf/cudf/core/dataframe.py	`89.54% <0.00%> (+<0.01%)`	⬆️
python/cudf/cudf/core/frame.py	`90.19% <0.00%> (+0.01%)`	⬆️
python/cudf/cudf/core/column/column.py	`88.69% <0.00%> (+0.05%)`	⬆️

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 79bd435...edcef21. Read the comment docs.

davidwendt

Looks good.

davidwendt · 2020-07-22T14:23:56Z

cpp/include/cudf/transform.hpp

+ * The result column is such that keys[result[i]] == input[i],
+ * where `keys` is the set of distinct values in `input` in sorted order.
+ *
+ * Examples:


Surround examples with @code{.pseudo} and @endcode doxygen tags.
https://github.com/rapidsai/cudf/blob/branch-0.15/cpp/docs/DOCUMENTATION.md#inline-examples

jrhemstad · 2020-06-25T00:52:47Z

cpp/include/cudf/transform.hpp

+ * output: [{1, 2, 3, 9}, {0, 2, 0, 1, 3}]
+ *
+ * @param input        Column containing values to be encode
+ * @param mr           Device memory resource used to allocate the returned bitmask.


Suggested change

* @param mr Device memory resource used to allocate the returned bitmask.

* @param mr Device memory resource used to allocate the returned column's device memory

jrhemstad · 2020-06-25T00:55:50Z

cpp/include/cudf/transform.hpp

+ * The encoded values are integers in the range [0, n), where `n`
+ * is the number of distinct values in the input column.
+ * The result column is such that keys[result[i]] == input[i],
+ * where `keys` is the set of distinct values in `input` in sorted order.


Suggested change

* where `keys` is the set of distinct values in `input` in sorted order.

* where `keys` is the set of distinct values in `input` in sorted ascending order.

jrhemstad · 2020-07-22T14:36:49Z

cpp/include/cudf/transform.hpp

+/**
+ * @brief Encode the values of the given column as integers
+ *
+ * The encoded values are integers in the range [0, n), where `n`


Describe the behavior for nulls in input.

Just added a description - let me know if it's clear enough

Ah but it's not really accurate.

rgsl888prabhu

A small doc change, other than that LGTM

rgsl888prabhu · 2020-07-22T14:35:20Z

cpp/include/cudf/detail/transform.hpp

@@ -52,5 +52,11 @@ std::pair<std::unique_ptr<rmm::device_buffer>, cudf::size_type> bools_to_mask(
  column_view const& input,
  rmm::mr::device_memory_resource* mr = rmm::mr::get_default_resource(),
  cudaStream_t stream                 = 0);
+


Nice catch -- fixed

Addressed.

shwina added 3 commits June 24, 2020 08:07

Add cudf::detail::codify and cudf::codify

7232c73

codify docs

cc44957

Add basic codify tests

1f4ebe8

shwina commented Jun 24, 2020

View reviewed changes

cpp/src/transform/codify.cpp Outdated Show resolved Hide resolved

harrism previously requested changes Jun 25, 2020

View reviewed changes

cpp/include/cudf/transform.hpp Outdated Show resolved Hide resolved

cpp/include/cudf/transform.hpp Outdated Show resolved Hide resolved

shwina and others added 2 commits June 25, 2020 07:10

Update cpp/include/cudf/transform.hpp

3fe07a6

Co-authored-by: Mark Harris <[email protected]>

Update cpp/include/cudf/transform.hpp

337e30c

Co-authored-by: Mark Harris <[email protected]>

shwina mentioned this pull request Jul 9, 2020

[DISCUSSION] Replacing na_sentinel with null mask for Series.label_encoding #5622

Closed

shwina added 3 commits July 21, 2020 13:43

Merge branch 'branch-0.15' of https://github.com/rapidsai/cudf into a…

f640bff

…dd-codify

Add more tests for codify

808570d

Rename codify->encode

a95c1e6

shwina marked this pull request as ready for review July 21, 2020 19:58

shwina requested review from a team as code owners July 21, 2020 19:58

shwina requested review from harrism and rgsl888prabhu July 21, 2020 19:58

Changelog

5ed85a7

shwina changed the title ~~[WIP] Add cudf::codify~~ [REVIEW] Add cudf::codify Jul 21, 2020

shwina changed the title ~~[REVIEW] Add cudf::codify~~ [REVIEW] Add cudf::encode Jul 21, 2020

davidwendt requested changes Jul 21, 2020

View reviewed changes

shwina and others added 7 commits July 21, 2020 16:35

Update cpp/tests/CMakeLists.txt

87ebf66

Co-authored-by: David <[email protected]>

Update cpp/src/transform/encode.cpp

dbde015

Co-authored-by: David <[email protected]>

Update cpp/src/transform/encode.cpp

336bee8

Co-authored-by: David <[email protected]>

Update cpp/tests/encode/encode_tests.cpp

d7bcde2

Co-authored-by: David <[email protected]>

Remove unused #include

b619bea

codify->encode

03b8bd9

Merge branch 'add-codify' of github.com:shwina/cudf into add-codify

2d532f1

davidwendt approved these changes Jul 22, 2020

View reviewed changes

Add code/endcode in cudf::encode doxygen examples

4569077

jrhemstad requested changes Jul 22, 2020

View reviewed changes

rgsl888prabhu approved these changes Jul 22, 2020

View reviewed changes

shwina added 4 commits July 22, 2020 10:40

Fix description of mr

7d670e9

Explain null behaviour in cudf::encode

07ed5d7

Doc for detail::encode

c163150

Fix null behaviour explanation

edcef21

jrhemstad approved these changes Jul 22, 2020

View reviewed changes

jrhemstad merged commit 66c5810 into rapidsai:branch-0.15 Jul 22, 2020

shwina mentioned this pull request Jul 22, 2020

[FEA] Add a libcudf codify function #5498

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] Add `cudf::encode` #5572

[REVIEW] Add `cudf::encode` #5572

shwina commented Jun 24, 2020 •

edited

Loading

GPUtester commented Jun 24, 2020

harrism left a comment

shwina commented Jun 25, 2020 •

edited

Loading

shwina commented Jun 25, 2020

davidwendt Jul 21, 2020

shwina Jul 21, 2020

codecov bot commented Jul 21, 2020 •

edited

Loading

davidwendt left a comment

davidwendt Jul 22, 2020

shwina Jul 22, 2020

jrhemstad Jun 25, 2020

shwina Jul 22, 2020

jrhemstad Jun 25, 2020

shwina Jul 22, 2020

jrhemstad Jul 22, 2020

shwina Jul 22, 2020

shwina Jul 22, 2020

shwina Jul 22, 2020

rgsl888prabhu left a comment

rgsl888prabhu Jul 22, 2020

shwina Jul 22, 2020

	* @param mr Device memory resource used to allocate the returned bitmask.
	* @param mr Device memory resource used to allocate the returned column's device memory

	* where `keys` is the set of distinct values in `input` in sorted order.
	* where `keys` is the set of distinct values in `input` in sorted ascending order.

[REVIEW] Add cudf::encode #5572

[REVIEW] Add cudf::encode #5572

Conversation

shwina commented Jun 24, 2020 • edited Loading

GPUtester commented Jun 24, 2020

harrism left a comment

Choose a reason for hiding this comment

shwina commented Jun 25, 2020 • edited Loading

shwina commented Jun 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 21, 2020 • edited Loading

Codecov Report

davidwendt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgsl888prabhu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[REVIEW] Add `cudf::encode` #5572

[REVIEW] Add `cudf::encode` #5572

shwina commented Jun 24, 2020 •

edited

Loading

shwina commented Jun 25, 2020 •

edited

Loading

codecov bot commented Jul 21, 2020 •

edited

Loading