Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[oneDPL] Histogram APIs #546

Closed
wants to merge 27 commits into from
Closed

[oneDPL] Histogram APIs #546

wants to merge 27 commits into from

Conversation

danhoeflinger
Copy link
Contributor

@danhoeflinger danhoeflinger commented Jun 17, 2024 •
edited
Loading

Proposing oneapi::dpl::histogram specification for oneDPL.

@danhoeflinger danhoeflinger changed the title Dev/dhoeflin/one dpl histogram [oneDPL] Histogram Jun 17, 2024
@danhoeflinger danhoeflinger changed the title [oneDPL] Histogram [oneDPL] Histogram APIs Jun 17, 2024
@akukanov akukanov added the DPL label Aug 12, 2024
akukanov
akukanov reviewed Aug 12, 2024
View reviewed changes
source/elements/oneDPL/source/parallel_api.rst Outdated Show resolved Hide resolved
source/elements/oneDPL/source/parallel_api.rst Outdated Show resolved Hide resolved
source/elements/oneDPL/source/parallel_api.rst Outdated Show resolved Hide resolved
source/elements/oneDPL/source/parallel_api.rst Outdated Show resolved Hide resolved
source/elements/oneDPL/source/parallel_api.rst Outdated Show resolved Hide resolved
source/elements/oneDPL/source/parallel_api.rst Outdated Show resolved Hide resolved
akukanov
akukanov reviewed Aug 16, 2024
View reviewed changes
source/elements/oneDPL/source/parallel_api.rst Outdated Show resolved Hide resolved
source/elements/oneDPL/source/parallel_api.rst Outdated Show resolved Hide resolved
source/elements/oneDPL/source/parallel_api.rst Outdated Show resolved Hide resolved
source/elements/oneDPL/source/parallel_api.rst Outdated Show resolved Hide resolved
akukanov
akukanov reviewed Aug 16, 2024
View reviewed changes
source/elements/oneDPL/source/parallel_api.rst Outdated Show resolved Hide resolved
source/elements/oneDPL/source/parallel_api.rst Outdated Show resolved Hide resolved
source/elements/oneDPL/source/parallel_api.rst Outdated Show resolved Hide resolved
source/elements/oneDPL/source/parallel_api.rst Outdated Show resolved Hide resolved
source/elements/oneDPL/source/parallel_api.rst Outdated Show resolved Hide resolved
akukanov
akukanov reviewed Aug 16, 2024
View reviewed changes
Copy link
Contributor

@akukanov akukanov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also add the description of the return value.

@danhoeflinger
Copy link
Contributor Author

danhoeflinger commented Aug 16, 2024 •
edited
Loading

Let's also add the description of the return value.

added.

danhoeflinger and others added 20 commits August 20, 2024 14:23
@danhoeflinger
first draft histogram spec
4a5dd2d 
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger
more explicit language / code
477f203 
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger
simplifying and combining overloads
82032db 
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger
formatting
fb2cfc8 
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger
removing extra space
7c97cf5 
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger
formatting
f25a685 
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger
semicolon
add3a6c 
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger
minor wording change
d17d582 
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger
wording
e790069 
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger @akukanov
update language in introduction
98e1628 
Co-authored-by: Alexey Kukanov <[email protected]>
@danhoeflinger
removing random access iterator explicit req
0175f31 
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger
fixing overflow of bins requirement
11d0ae5 
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger
improving the language on types, sortedness, and definition of mapping
16aec2c 
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger
using operator<=
f14756a 
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger
renaming boundary variables for clarity
10e5377 
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger
formatting
5b099f3 
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger
removing explicit requirement for output data allocation
1a91065 
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger
adding information about the number of bins for each overload
5fcec63 
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger
num_bins -> num_intervals
e0c54bc 
Signed-off-by: Dan Hoeflinger <[email protected]>
@danhoeflinger @akukanov
Apply suggestions from code review
281adca 
Co-authored-by: Alexey Kukanov <[email protected]>
timmiesmith
timmiesmith reviewed Aug 23, 2024
View reviewed changes
source/elements/oneDPL/source/parallel_api.rst Outdated
@@ -519,5 +519,40 @@ than an element in the range being searched.

The elements of ``[start, end)`` must be partitioned with respect to the comparator used.

.. code:: cpp

template <typename Policy, typename InputIt, typename Size, typename OutputIt,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest swapping the order of OutputIt and ValueType to be consistent with their use in the API.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you have caught something here that is a problem.
I have a proposal to fix it, but I think we want to do more than just reorder the template parameters.

Instead of having a free floating ValueType template parameter. I believe we should instead directly use std::iterator_traits<InputIt>::value_type as the type of first_interval_begin and last_interval_end.

I think it is better for force users to provide bounds which are convertible to the value_type of the input sequence, and do the conversion once when accepting the arguments rather than each time in the comparison operator. This will change the signature and some of the language. I have updated it accordingly, please take a look.

Also, while this is not the same exact API as what is currently implemented in oneDPL, it is not incompatible with the existing implementation's API. I believe we can continue to support that API as an extension without a breaking change, but lets discuss that in the implementation PR, which will be up shortly.

@danhoeflinger danhoeflinger mentioned this pull request Aug 23, 2024
Closed
@danhoeflinger
align numbers for visibility
04e62ff 
Signed-off-by: Dan Hoeflinger <[email protected]>
This was referenced Aug 28, 2024
Merged
Closed
@danhoeflinger
Copy link
Contributor Author

We have decided to instead go with #571 in an offline discussion. closing this.

@danhoeflinger danhoeflinger mentioned this pull request Sep 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akukanov akukanov akukanov approved these changes

@timmiesmith timmiesmith timmiesmith left review comments

Assignees
No one assigned
Labels
DPL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@danhoeflinger @timmiesmith @akukanov