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.

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

[Doc] Split radix_sort doc page into key and key-value API #1586

Merged
merged 15 commits into from
Jul 4, 2024

Conversation

dmitriy-sobolev
Copy link
Contributor

@dmitriy-sobolev dmitriy-sobolev commented May 15, 2024

Separate radix_sort and radix_sort_by_key.

Pros:

  • Improved readability: no branching for a specific case, less number of interfaces per page.
  • Examples do not take too much space.

Cons:

  • Duplication, more to maintain.

I assume that benefits outweigh drawbacks specifically from a user perspective.

This PR was motivated by this discussion: #1583 (comment)

@dmitriy-sobolev dmitriy-sobolev changed the title [Doc] Split radix_sort and radix_sort_by_key [Doc] Split radix_sort doc page into key and key-value API May 15, 2024
@dmitriy-sobolev dmitriy-sobolev force-pushed the dev/dmitriy-sobolev/doc-esimd-split branch from c64c223 to e1b48f0 Compare May 30, 2024 13:34
@dmitriy-sobolev dmitriy-sobolev requested review from danhoeflinger and removed request for danhoeflinger May 31, 2024 09:56
@dmitriy-sobolev dmitriy-sobolev marked this pull request as ready for review May 31, 2024 09:57
Signed-off-by: Dmitriy Sobolev <[email protected]>
danhoeflinger
danhoeflinger previously approved these changes Jun 6, 2024
Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

LGTM,
I have one optional comment about wording preference, which I will leave up to you. Thanks for making this change, I think it makes it more readable, even if it requires some duplication.

Signed-off-by: Dmitriy Sobolev <[email protected]>
danhoeflinger
danhoeflinger previously approved these changes Jun 6, 2024
Signed-off-by: Dmitriy Sobolev <[email protected]>
@dmitriy-sobolev
Copy link
Contributor Author

@dcbenito, could you assist with verifying the changes?

danhoeflinger
danhoeflinger previously approved these changes Jun 20, 2024
dcbenito
dcbenito previously approved these changes Jun 24, 2024
Copy link
Contributor

@dcbenito dcbenito left a comment

Choose a reason for hiding this comment

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

Minor changes, otherwise LGTM.


where N\ :sub:`keys_per_workgroup` and N\ :sub:`values_per_workgroup` are the amounts of memory
to store keys and values, respectively. `C` is some additional space for storing internal data.
where N\ :sub:`keys_per_workgroup` is the amounts of memory to store keys.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change amounts to amount.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

- The element type of sequence(s) to sort must be a C++ integral or floating-point type
other than ``bool`` with a width of up to 64 bits.

.. note::
Copy link
Contributor

Choose a reason for hiding this comment

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

This formatting is odd and not rendering in Note form. Should this be a note or a header?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I got you correctly, the comment is about rendering of the note if you open the file on GitHub in "preview" format.
If so, there are other instances of the same issue in other places, for example, https://github.com/oneapi-src/oneDPL/blob/main/documentation/library_guide/parallel_api/range_based_api.rst, so I guess it is a common issue.
The note is rendered properly if this document is interpreted into html format by Sphinx tool.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the Current Limitations sub-header was confusing me. I built the docs and can see that it does render correctly, so this is a non-issue that you can ignore.

N\ :sub:`keys_per_workgroup` + N\ :sub:`values_per_workgroup` + C

where N\ :sub:`keys_per_workgroup` and N\ :sub:`values_per_workgroup` are the amounts of memory
to store keys and values, respectively. `C` is some additional space for storing internal data.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the double space between "...respectively. 'C' is some..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

@dmitriy-sobolev dmitriy-sobolev dismissed stale reviews from dcbenito and danhoeflinger via f16b5d9 June 26, 2024 12:25
@dmitriy-sobolev dmitriy-sobolev merged commit ad150a6 into main Jul 4, 2024
19 of 20 checks passed
@dmitriy-sobolev dmitriy-sobolev deleted the dev/dmitriy-sobolev/doc-esimd-split branch July 4, 2024 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants