[REVIEW] Replace function #106
Conversation
|
Can one of the admins verify this patch? |
|
ok to test |
|
API seems reasonable to me |
| @@ -0,0 +1,110 @@ | |||
| /* | |||
There was a problem hiding this comment.
This should be moved to a separate bench folder as we discussed in the binary ops PR review?
There was a problem hiding this comment.
And separated into benchmarks and unit tests?
| @@ -0,0 +1,143 @@ | |||
| /* | |||
There was a problem hiding this comment.
Should add more documentation/comments to the tests
There was a problem hiding this comment.
@nsakharnykh Can you approve your review since this is resolved?
…s to replace-test
…s to replace-test
include/gdf/cffi/functions.h
Outdated
| /// \param[in] values contains the replacement values | ||
| /// | ||
| /// Note that `to_replace` and `values` are related by the index | ||
| gdf_error gdf_replace(gdf_column * column, |
There was a problem hiding this comment.
The interface is confusing to me. Why does it have column and to_replace parameters? What happens to column? What happens to to_replace? Why not just make a gdf_copy(out_column, in_column)?
There was a problem hiding this comment.
Now I see. The meaning of "replace" is not clear. It's not a copy. The semantics are actually those of "find_and_replace_all": For each value in to_replace, find all instances of that value in column and replace it with the corresponding value in values. This should be made clear in the header documentation for the function. Consider changing the name to gdf_find_and_replace_all() or something like that...
| @@ -0,0 +1,43 @@ | |||
|
|
|||
There was a problem hiding this comment.
We're adding google benchmark in this PR? Shouldn't that be orthogonal to gdf_replace, and therefore a separate PR?
There was a problem hiding this comment.
Since this is the first benchmark I've seen added to libgdf...
There was a problem hiding this comment.
Since no other PR depends of this, I put google benchmark here. But I could make another PR if it's relevant.
src/replace.cu
Outdated
| if (NotSameDType(column, to_replace, values)) { return GDF_CUDA_ERROR; } | ||
|
|
||
| switch (column->dtype) { | ||
| #define WHEN(DTYPE) \ |
There was a problem hiding this comment.
I don't really like calling this macro "WHEN" -- it's not descriptive. Something like REPLACE_CASE would be clearer. Then you would have
switch (column->type) {
REPLACE_CASE(INT8);
REPLACE_CASE(INT16);
// etc.
}
mike-wendt
added
4 - Needs Rework
API proposal for the replace function.