[oneDPL] Transform_if APIs #547
Conversation
| @@ -688,5 +688,37 @@ than an element in the range being searched. | |||
|
|
|||
| The elements e of [start, end) must be partitioned with respect to the comparator used. | |||
There was a problem hiding this comment.
Should e be in quotes? Should the brackets be the same? Right now, it is mixed with squares/parentheses.
There was a problem hiding this comment.
This e thing should be removed, but it's better done in a separate patch.
The brackets are intentionally different, it is the mathematical notation for so-called half-intervals.
There was a problem hiding this comment.
Thank you! I did not know that about the brackets and will keep that in mind for future reviews.
There was a problem hiding this comment.
This e thing should be removed, but it's better done in a separate patch.
Also added to #565.
| transform_if(Policy&& policy, InputIt1 start1, InputIt1 end1, InputIt2 start2, OutputIt result, | ||
| BinaryOp op, BinaryPredicate pred); // (2) | ||
|
|
||
| :code:`oneapi::dpl::transform_if` applies a given function to the elements of the input sequence(s) that |
There was a problem hiding this comment.
Why are we using :code:'' in this file and "" in other files? I think that we should standardize this and am partial to "".
Please add a colon after "...the algorithm"
There was a problem hiding this comment.
for :code: vs , I don't have a good explanation other than staying consistent with the other cases in this file. If we want to make a formatting change on this topic, I'd suggest we do it for the whole oneDPL spec, and in a separate PR from this.
There was a problem hiding this comment.
After a quick look around, I think that this file is the only one using :code: (for docs anyway). I would prefer that we stay consistent. I think that a new PR for this issue would be good to fix before the next release.
There was a problem hiding this comment.
To be clear, this is the oneDPL specification within the oneAPI specification in UXL, rather than docs which would be tied to an intel release.
I see a few other instances of :code: in sycl_kernels_api.rst and common.rst, but most are here in this file. However, I agree it would be good to update with a PR prior to the specification freeze.
There was a problem hiding this comment.
That is good to keep in mind!
Would you like me to create a PR? I can have that done by EOD.
There was a problem hiding this comment.
At the HTML level, this is the output for backquotes:
<code class="docutils literal notranslate">
<span class="pre">using</span> <span class="pre">scalar_type</span> <span class="pre">=</span> <span class="pre">result_type;</span>
</code>
and this is the output for :code:
<code class="code docutils literal notranslate">
<span class="pre">namespace</span> <span class="pre">oneapi::dpl::execution</span>
</code>
(the examples use different strings - I have not found the same or similar strings for both styles.)
The difference seems to be only in the extra "code" class name in attributes.. I guess it technically allows to set different formatting in CSS or JavaScript, but I do not know if it is used in the oneAPI specification. Visually both styles look the same.
There was a problem hiding this comment.
I looked through the other products in this repo and we are the only ones using the :code:'value' (single quote)and only in some spots. Most of the instances are "value"(no :code: and with double quotes). We should stick to one.
I have created a PR, but do not have approval/access to push it. Can someone on this thread give me write access for this repo?
There was a problem hiding this comment.
As far as I can tell, contributions/PRs to this repo are done from forks, not from repo branches.
If you do not want to fork it in your own GitHub account, I can create a PR from my fork.
There was a problem hiding this comment.
I have followed up via email. Thank you @akukanov
| :code:`oneapi::dpl::transform_if` applies a given function to the elements of the input sequence(s) that | ||
| satisfy a given predicate, and stores the result to the output. Depending on the arguments, the algorithm | ||
|
|
||
| 1. evaluates the unary predicate :code:`pred` for each position :code:`i` of the sequence |
There was a problem hiding this comment.
Please capitalize evaluates in steps 1 and 2.
There was a problem hiding this comment.
both are continuing a sentence started prior to the enumerated list, should we still capitalize them? Or do you think we should reformat the structure to avoid that?
There was a problem hiding this comment.
This is a weird one where normal grammar rules don't really apply. Per our and IDZ's style guide we format the text using sentence case, this is why the capital for "evaluates". We also use a sentence or snippet to introduce the list, so "Depending on the arguments, the algorithm:" is the intro and why we want a colon after "algorithm".
There was a problem hiding this comment.
alright I've applied that pair of changes.
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
Signed-off-by: Dan Hoeflinger <[email protected]>
danhoeflinger
force-pushed
the
dev/dhoeflin/oneDPL_transform_if
branch
from
b808ab6
to
fdd74ff
Compare
There was a problem hiding this comment.
The description looks good to me. I'll follow the changes in #565 to continue the discussion.
Proposing
oneapi::dpl::transform_ifspecification for oneDPL.