Add vectorized_sort function - #2431
Conversation
for more information, see https://pre-commit.ci
|
I have written some pytest tests, but would wait until #777 is merged, to write them into the |
- Fallback onto `sort` for one dimensional arrays.
#2386 was actually already merged. Don't worry about this now. I just love this new feature and want to advertise it ;) |
brownbaerchen
left a comment
There was a problem hiding this comment.
It's a bit too hot for me to understand everything. I marked a bunch of small things that could be a bit cleaner. I suggest you add some tests and then I'll have another look.
Since we want to extract the side effect of sorting from unique, a comparison with unique would be a useful test.
|
I failed at creating a stack of this and #2332, which depends on this. Getting the commit histories in order after the fact is not so easy :D I'll take care of fixing the merge conflicts that will arise when we merge #2332 after merging this. But on the bright side, the tests seem to pass :) Sorry about the mess. |
- Added tests for `vectorized_sort`
for more information, see https://pre-commit.ci
Co-authored-by: Thomas Saupe <39156931+brownbaerchen@users.noreply.github.com>
Removed unnecessary distinction between ranks. Co-authored-by: Thomas Saupe <39156931+brownbaerchen@users.noreply.github.com>
Co-authored-by: Thomas Saupe <39156931+brownbaerchen@users.noreply.github.com>
Co-authored-by: Thomas Saupe <39156931+brownbaerchen@users.noreply.github.com>
for more information, see https://pre-commit.ci
Co-authored-by: Thomas Saupe <39156931+brownbaerchen@users.noreply.github.com>
for more information, see https://pre-commit.ci
…ing' into 363-vectorized-sorting
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2431 +/- ##
==========================================
+ Coverage 91.74% 91.79% +0.04%
==========================================
Files 87 87
Lines 14168 14278 +110
==========================================
+ Hits 12998 13106 +108
- Misses 1170 1172 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
|
There was the Idea to also implement a |
|
The other Idea was to use |
Due Diligence
Description
Issue/s resolved: #363
Changes proposed:
Introduce the vectorized sorting mentioned in #363, which is also a lexicographical sort
Allows to finalize #2332.
Type of change
Memory requirements
A complete alltoallv requires a total of 3x the memory here: the original buffer, contiguous send buffer and the receive buffer.
One could in theory implement an own chunking algorithm to reduce this, if needed. That might make the performance worse or better. Testing is the only way to know for sure.
Custom datatypes should also be considered, but would need extensive analysis on the MPI implementation itself, since the sending rank might want to send multiple packets and might need to pack them together, creating again a copy, unless the network protocol can completely handle the non-contiguous data.
Performance
Does this change modify the behaviour of other functions? If so, which?
no