rapidsai / node

GPU-accelerated data science and visualization in node
https://rapidsai.github.io/node/
Apache License 2.0
187 stars 20 forks source link

Add StringSeries.concatenate #309

Closed bryevdv closed 3 years ago

bryevdv commented 3 years ago

Pre-req for hypergraph implementation.

The Python "counterpart" for this would be __add__ which does not exist on the JS side, so I have simply mirrored the cudf::strings::concatenate API from libcudf as a static StringSeries.concatenate. Open to suggestion for better naming since there is already, e.g. concat for other things.

AjayThorve commented 3 years ago

looks good, but I can see the naming being a bit confusing although there are enough unique factors as well.

concat is applied to a stringSeries, whereas concatenate is a standalone function that accepts multiple stringSeries as a list? I think that should be fine

trxcllnt commented 3 years ago

what about StringSeries.append() and StringSeries.prepend()? Or do we already have those?

edit: on second thought, concatenate seems appropriate since the input is an entire Table of String columns.

bryevdv commented 3 years ago

@trxcllnt right concatenate is nice because you can pass any number of string columns at once. But it would be trivial to define prepend and append as convenience helpers if we wanted to

 public prepend(other: StringSeries,
                opts: ConcatenateOptions = {}): Series<Utf8String> {
    return StringSeries.concatenate([other, this], opts)
}

I guess I am +0 on that.

@AjayThorve exactly right. I suppoe if we wanted to be more explicit then concatenate_string_columns but that name seems a bit redundant given the method only exists on StringSeries

bryevdv commented 3 years ago

@trxcllnt says "ship it" so I will merge when green

bryevdv commented 3 years ago

Uhh

Screen Shot 2021-10-06 at 17 39 11
bryevdv commented 3 years ago

looks like it did merge! 🤷