Feature/radix sort #1867

oleksandr-pavlyk · 2024-10-17T20:50:35Z

This PR implements radix sort, and exposes it via new kind keyword of dpt.sort and dpt.argsort functions.
Supported values of kind keyword are "stable", "radixsort", and "mergesort", with the default value being None (same as "stable"). The "stable" kind uses radix sort for boolean and short integral types,

Have you provided a meaningful PR description?
Have you added a test, reproducer or referred to an issue with a reproducer?
Have you tested your changes locally for CPU and GPU devices?
Have you made sure that new changes do not introduce compiler warnings?
Have you checked performance impact of proposed changes?
Have you added documentation for your changes, if necessary?
Have you added your changes to the changelog?
If this PR is a work in progress, are you opening the PR as a draft?

…n of radix sort

Add common typedef for sort_contig_fn_ptr_t from merge_sort header and radix_sort header files into a new file. Used it in cpp files.

Supported values for kind are "radixsort", "mergesort", "stable". The default is None (same as "stable"). For stable, radix sort is used for bool, (u)int8, (u)int16. Radix sort is supported for integral, boolean and real floating point types.

github-actions · 2024-10-17T21:33:14Z

View rendered docs @ https://intelpython.github.io/dpctl/pulls/1867/index.html

github-actions · 2024-10-17T21:40:21Z

Array API standard conformance tests for dpctl=0.19.0dev0=py310hdf72452_145 ran successfully.
Passed: 894
Failed: 1
Skipped: 119

coveralls · 2024-10-17T21:42:08Z

coverage: 87.705% (+0.02%) from 87.686%
when pulling 09236c9 on feature/radix-sort
into 7ffcca4 on master.

oleksandr-pavlyk · 2024-10-17T21:54:49Z

Build with DPCTL_TARGET_CUDA=ON succeeds, test suite run on RTX 3050 passed.

github-actions · 2024-10-21T04:04:08Z

Array API standard conformance tests for dpctl=0.19.0dev0=py310hdf72452_149 ran successfully.
Passed: 894
Failed: 1
Skipped: 119

github-actions · 2024-10-22T14:54:09Z

Array API standard conformance tests for dpctl=0.19.0dev0=py310hdf72452_158 ran successfully.
Passed: 894
Failed: 1
Skipped: 119

ndgrigorian · 2024-10-28T20:37:32Z

dpctl/dpctl/tensor/libtensor/include/kernels/sorting/merge_sort.hpp

Line 809 in 3c05c1b

static constexpr size_t determine_automatically = 0;

This line and the initial sorted_block_size value seem to be unused.

static constexpr size_t determine_automatically = 0;
size_t sorted_block_size =
    (sort_nelems >= 512) ? 512 : determine_automatically;

True or not, in merge_sort_detail::sort_over_work_group_contig_impl the sorted_block_size is set to the same value with no check against the current value of sorted_block_size. So these lines can seemingly be removed entirely.

…eter The intent is to reduce the build time, build memory footprint, and binary size of the sorting_impl module. With this change it stands at 46MB, before it was 72MB.

…x_impl With this change, _tensor_sorting_impl goes back to 17MB, and _tensor_sorting_radix_impl is 30MB. The memory footprint of linking should be greatly reduced, speeding up the building process, reducing the required memory footprint, and providing better parallelisation opportunities for the build job. The build time on my Core i7 reduced from 45 minutes to 33 minutes.

…late-parameter-to-an-argument Change descending from template parameter to an argument

github-actions · 2024-10-29T19:02:56Z

Array API standard conformance tests for dpctl=0.19.0dev0=py310hdf72452_174 ran successfully.
Passed: 894
Failed: 1
Skipped: 119

dpctl/tensor/_sorting.py

ndgrigorian · 2024-11-01T21:49:44Z

Nit: renaming of sort.cpp/sort.hpp and argsort.cpp/argsort.hpp to merge_sort and merge_argsort may be good for clarity.

dpctl/tensor/libtensor/source/sorting/py_argsort_common.hpp

dpctl/tensor/libtensor/source/sorting/py_sort_common.hpp

dpctl/tensor/libtensor/include/kernels/sorting/radix_sort.hpp

sort.cpp -> merge_sort.cpp, argsort.cpp -> merge_argsort.cpp Refined exception texts thrown when implementation function pointer is found missing.

github-actions · 2024-11-04T00:18:21Z

Array API standard conformance tests for dpctl=0.19.0dev0=py310hdf72452_188 ran successfully.
Passed: 894
Failed: 1
Skipped: 119

oleksandr-pavlyk · 2024-11-04T01:50:20Z

@ndgrigorian I have pushed changes to address all the issues highlighted in review.

ndgrigorian

This LGTM now, I've run some benchmarks for different array sizes and shapes and run into no problems.

Thank you @oleksandr-pavlyk !

dpctl/tensor/libtensor/include/kernels/sorting/radix_sort.hpp

github-actions · 2024-11-06T14:12:40Z

Array API standard conformance tests for dpctl=0.19.0dev0=py310hdf72452_189 ran successfully.
Passed: 895
Failed: 0
Skipped: 119

ndgrigorian · 2024-11-06T23:46:53Z

dpctl/tensor/libtensor/include/kernels/sorting/radix_sort.hpp

+
+template <typename argTy, typename IndexTy>
+sycl::event
+radix_argsort_axis1_contig_impl(sycl::queue &exec_q,


This impl function seems to be unused anywhere. Should it be kept around or removed as dead code?

oleksandr-pavlyk added 9 commits October 16, 2024 12:35

Renamed some files in sorting folders, in preparation for introductio…

830f428

…n of radix sort

Add header file implementing radix sort

a6d51af

Add a comment next to radix_sort_contig_fn_ptr_t type definition

3517745

Added Python API to exercise radix sort functions

dbe62d0

Add license headers

6d33867

Add common typedef for sort_contig_fn_ptr_t from merge_sort header and radix_sort header files into a new file. Used it in cpp files.

Rename argument variable from stable_sort_fns to sort_fns

e274c2d

Add Python API to check if radix sort is supported for given dtype

c2f8486

Parametrize sorting tests by kind

2684651

oleksandr-pavlyk marked this pull request as ready for review October 18, 2024 21:34

oleksandr-pavlyk requested a review from ndgrigorian as a code owner October 18, 2024 21:34

oleksandr-pavlyk added 2 commits October 20, 2024 22:14

Use sycl_free_noexcept

662b46b

Remove unused include statement

fef0fe4

Add entry to changelog about radix sort algorithm

3c05c1b

oleksandr-pavlyk requested a review from antonwolfy October 25, 2024 17:29

oleksandr-pavlyk added 3 commits October 29, 2024 09:29

Change to pass sorting direction as call argument, not template param…

bbe1019

…eter The intent is to reduce the build time, build memory footprint, and binary size of the sorting_impl module. With this change it stands at 46MB, before it was 72MB.

Merge pull request #1883 from IntelPython/change-descending-from-temp…

d63dd70

…late-parameter-to-an-argument Change descending from template parameter to an argument

ndgrigorian reviewed Oct 31, 2024

View reviewed changes

dpctl/tensor/_sorting.py Show resolved Hide resolved

ndgrigorian reviewed Nov 1, 2024

View reviewed changes

dpctl/tensor/libtensor/source/sorting/py_argsort_common.hpp Outdated Show resolved Hide resolved

ndgrigorian reviewed Nov 1, 2024

View reviewed changes

dpctl/tensor/libtensor/source/sorting/py_sort_common.hpp Show resolved Hide resolved

ndgrigorian reviewed Nov 1, 2024

View reviewed changes

dpctl/tensor/libtensor/include/kernels/sorting/radix_sort.hpp Outdated Show resolved Hide resolved

oleksandr-pavlyk added 3 commits November 3, 2024 17:31

Address PR feedback

446ce05

sort.cpp -> merge_sort.cpp, argsort.cpp -> merge_argsort.cpp Refined exception texts thrown when implementation function pointer is found missing.

Renamed n_values->n_to_sort for readability per review

93db58a

Merge remote-tracking branch 'origin/master' into feature/radix-sort

350738b

ndgrigorian previously approved these changes Nov 4, 2024

View reviewed changes

ndgrigorian reviewed Nov 4, 2024

View reviewed changes

dpctl/tensor/libtensor/include/kernels/sorting/radix_sort.hpp Outdated Show resolved Hide resolved

Use sycl_free_noexcept instead of sycl::free

09236c9

oleksandr-pavlyk dismissed ndgrigorian’s stale review via 09236c9 November 6, 2024 13:29

oleksandr-pavlyk mentioned this pull request Nov 6, 2024

Delete remove defaults channel step from test_windows job #1886

Open

8 tasks

ndgrigorian reviewed Nov 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/radix sort #1867

Feature/radix sort #1867

oleksandr-pavlyk commented Oct 17, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024

github-actions bot commented Oct 17, 2024

coveralls commented Oct 17, 2024 •

edited

Loading

oleksandr-pavlyk commented Oct 17, 2024

github-actions bot commented Oct 21, 2024

github-actions bot commented Oct 22, 2024

ndgrigorian commented Oct 28, 2024

github-actions bot commented Oct 29, 2024

ndgrigorian commented Nov 1, 2024

github-actions bot commented Nov 4, 2024

oleksandr-pavlyk commented Nov 4, 2024

ndgrigorian left a comment

github-actions bot commented Nov 6, 2024

ndgrigorian Nov 6, 2024

Feature/radix sort #1867

Are you sure you want to change the base?

Feature/radix sort #1867

Conversation

oleksandr-pavlyk commented Oct 17, 2024 • edited Loading

github-actions bot commented Oct 17, 2024

github-actions bot commented Oct 17, 2024

coveralls commented Oct 17, 2024 • edited Loading

oleksandr-pavlyk commented Oct 17, 2024

github-actions bot commented Oct 21, 2024

github-actions bot commented Oct 22, 2024

ndgrigorian commented Oct 28, 2024

github-actions bot commented Oct 29, 2024

ndgrigorian commented Nov 1, 2024

github-actions bot commented Nov 4, 2024

oleksandr-pavlyk commented Nov 4, 2024

ndgrigorian left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 6, 2024

ndgrigorian Nov 6, 2024

Choose a reason for hiding this comment

oleksandr-pavlyk commented Oct 17, 2024 •

edited

Loading

coveralls commented Oct 17, 2024 •

edited

Loading