Expose stream parameter in public strings split/partition APIs #14247
Conversation
davidwendt
added
2 - In Progress
davidwendt
added
3 - Ready for Review
| * @param input Strings instance for this operation | ||
| * @param delimiter UTF-8 encoded string indicating where to split each string. | ||
| * Default of empty string indicates split on whitespace. | ||
| * @param mr Device memory resource used to allocate the returned table's device memory. | ||
| * @return New table of strings columns. | ||
| * @param stream CUDA stream used for device memory operations and kernel launches | ||
| * @param mr Device memory resource used to allocate the returned table's device memory | ||
| * @return New table of strings columns |
There was a problem hiding this comment.
Nit: It is a bit awkward to have mixed style like this: some sentences end with period while some others don't.
There was a problem hiding this comment.
What do you suggest for the delimiter parameter description? I've used semi-colon before but that was criticized as well.
There was a problem hiding this comment.
This is a difficult question too. IMO the best and most consistent way is to end every sentence with a period so we don't have situations like this. But since we are not using period, probably we should avoid having separate sentences for each parameter description. For example:
* @param delimiter UTF-8 encoded string indicating where to split each string,
* default of empty string indicates split on whitespace.
There was a problem hiding this comment.
The problem is these descriptions are not sentences and so do not warrant a period.
I'll go with a comma for now if that looks better though I'd prefer a semi-colon.
Unfortunately, we don't seem to have any consensus on this.
There was a problem hiding this comment.
I'll weigh in. :) Docs are writing that we would like to be readable, so we should apply sound style and grammar rules. If it's a sentence, end it with a period. I argue that parameter definitions should be sentences, even though we often leave out definite articles and pronouns and the verb ("to be") is often implied ("This CUDA stream is used for device memory operations and kernel launches."). Treating them all as sentences ensures consistent style when a parameter definition requires more than one sentence, as in this case.
I agree that semicolons are fine to use where appropriate. Dusting off my copy of Strunk & White, chapter 1:
(Rule 5) Do not join independent clauses with a comma. If two or more clauses grammatically complete and not joined by a conjunction are to form a single compound sentence, the proper mark of punctuation is a semicolon.
(Rule 6) Do not break sentences in two. In other words, don't use periods as commas.
Mr. White didn't give a rule for when to decide that two clauses should form a compound sentence, though he mentioned typical usage is "cause and consequence". Personally I think in the case of delimiter it makes sense as two sentences the way you have it.
| * @param input Strings instance for this operation | ||
| * @param delimiter UTF-8 encoded string indicating where to split each string. | ||
| * Default of empty string indicates split on whitespace. | ||
| * @param mr Device memory resource used to allocate the returned table's device memory. | ||
| * @return New table of strings columns. | ||
| * @param stream CUDA stream used for device memory operations and kernel launches | ||
| * @param mr Device memory resource used to allocate the returned table's device memory | ||
| * @return New table of strings columns |
There was a problem hiding this comment.
I'll weigh in. :) Docs are writing that we would like to be readable, so we should apply sound style and grammar rules. If it's a sentence, end it with a period. I argue that parameter definitions should be sentences, even though we often leave out definite articles and pronouns and the verb ("to be") is often implied ("This CUDA stream is used for device memory operations and kernel launches."). Treating them all as sentences ensures consistent style when a parameter definition requires more than one sentence, as in this case.
I agree that semicolons are fine to use where appropriate. Dusting off my copy of Strunk & White, chapter 1:
(Rule 5) Do not join independent clauses with a comma. If two or more clauses grammatically complete and not joined by a conjunction are to form a single compound sentence, the proper mark of punctuation is a semicolon.
(Rule 6) Do not break sentences in two. In other words, don't use periods as commas.
Mr. White didn't give a rule for when to decide that two clauses should form a compound sentence, though he mentioned typical usage is "cause and consequence". Personally I think in the case of delimiter it makes sense as two sentences the way you have it.
|
/merge |
Description
Follow on to PR #13997 which did not include all the split APIs or a stream test.
Add stream parameter to public APIs:
cudf::strings::partition()cudf::strings::rpartition()cudf::strings::split_re()cudf::strings::rsplit_re()cudf::strings::split_record_re()cudf::strings::rsplit_record_re()Also cleaned up some of the doxygen comments.
Reference #13744
Checklist