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

Add 'indirect_sort' #117

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Add 'indirect_sort' #117

wants to merge 7 commits into from

Conversation

mclow
Copy link
Collaborator

@mclow mclow commented Jun 15, 2023

No description provided.

Copy link

@Flamefire Flamefire left a comment

Choose a reason for hiding this comment

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

Looks potentially useful.

Added some ideas for improving the description and pointed out a few places for improvement/typo-fixing.

Comment on lines 12 to 14
There are times that you want a sorted version of a sequence, but for some reason or another, you don't really want to sort them. Maybe the elements in the sequence are non-copyable (or non-movable), or the sequence is const, or they're just really expensive to move around. An example of this might be a sequence of records from a database.

Nevertheless, you might want to sort them. That's where indirect sorting comes in. In a "normal" sort, the elements of the sequence to be sorted are shuffled in place. In indirect sorting, the elements are unchanged, but the sort algorithm returns to you a "permutation" of the elements that, when applied, will leave the elements in the sequence in a sorted order.

Choose a reason for hiding this comment

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

How about this a bit shorter wording especially avoiding to mention the need to sort twice:

Suggested change
There are times that you want a sorted version of a sequence, but for some reason or another, you don't really want to sort them. Maybe the elements in the sequence are non-copyable (or non-movable), or the sequence is const, or they're just really expensive to move around. An example of this might be a sequence of records from a database.
Nevertheless, you might want to sort them. That's where indirect sorting comes in. In a "normal" sort, the elements of the sequence to be sorted are shuffled in place. In indirect sorting, the elements are unchanged, but the sort algorithm returns to you a "permutation" of the elements that, when applied, will leave the elements in the sequence in a sorted order.
There are times that you want a sorted version of a sequence, but for some reason you don't want to modify it. Maybe the elements in the sequence can't be moved/copied, e.g. the sequence is const, or they're just really expensive to move around. An example of this might be a sequence of records from a database.
That's where indirect sorting comes in. In a "normal" sort, the elements of the sequence to be sorted are shuffled in place. In indirect sorting, the elements are unchanged, but the sort algorithm returns a "permutation" of the elements that, when applied, will put the elements in the sequence in a sorted order.

Are the double-spaces after each sentence intended?


Nevertheless, you might want to sort them. That's where indirect sorting comes in. In a "normal" sort, the elements of the sequence to be sorted are shuffled in place. In indirect sorting, the elements are unchanged, but the sort algorithm returns to you a "permutation" of the elements that, when applied, will leave the elements in the sequence in a sorted order.

Say you have a sequence `[first, last)` of 1000 items that are expensive to swap:

Choose a reason for hiding this comment

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

Suggested change
Say you have a sequence `[first, last)` of 1000 items that are expensive to swap:
Assume a sequence `[first, last)` of 1000 items that are expensive to swap:


#include <algorithm> // for std::sort (and others)
#include <functional> // for std::less
#include <vector> // for std:;vector

Choose a reason for hiding this comment

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

Typo:

Suggested change
#include <vector> // for std:;vector
#include <vector> // for std::vector

But is that comment really required?

///

#ifndef BOOST_ALGORITHM_IS_INDIRECT_SORT
#define BOOST_ALGORITHM_IS_INDIRECT_SORT

Choose a reason for hiding this comment

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

Unusual include guard. Why not BOOST_ALGORITHM_INDIRECT_SORT?

/// \fn indirect_sort (RAIterator first, RAIterator las )
/// \returns a permutation of the elements in the range [first, last)
/// such that when the permutation is applied to the sequence,
/// the result is sorted according to the predicate pred.

Choose a reason for hiding this comment

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

Suggested change
/// the result is sorted according to the predicate pred.
/// the result is sorted in non-descending order.

/// \param last The end of the input sequence
///
template <typename RAIterator>
std::vector<size_t> indirect_sort (RAIterator first, RAIterator last) {

Choose a reason for hiding this comment

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

Suggested change
std::vector<size_t> indirect_sort (RAIterator first, RAIterator last) {
Permutation indirect_sort (RAIterator first, RAIterator last) {



void test_sort () {
BOOST_CXX14_CONSTEXPR int num[] = { 1,3,5,7,9, 2, 4, 6, 8, 10 };

Choose a reason for hiding this comment

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

Suggested change
BOOST_CXX14_CONSTEXPR int num[] = { 1,3,5,7,9, 2, 4, 6, 8, 10 };
int num[] = { 1,3,5,7,9, 2, 4, 6, 8, 10 };

or int *first = &num[0]; is invalid isn't it?


// A permutation of size N is a sequence of values in the range [0..N)
// such that no value appears more than once in the permutation.
bool isa_permutation(Permutation p, size_t N) {

Choose a reason for hiding this comment

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

Suggested change
bool isa_permutation(Permutation p, size_t N) {
bool is_a_permutation(Permutation p, size_t N) {

is more readable.

test_one_sort(v.begin(), v.end(), std::greater<int>());
}

BOOST_AUTO_TEST_CASE( test_main )

Choose a reason for hiding this comment

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

Why that extra method and not using BOOST_AUTO_TEST_CASE(test_sort) directly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because I expect there to be more test cases in the future.

Choose a reason for hiding this comment

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

But the whole idea of BOOST_AUTO_TEST_CASE is that you simply "decorate" each test case with that and NOT have a "main" function. By default it will run each such function sequentially even allowing you to filter test based on their name from the CLI.

-->

BOOST_AUTO_TEST_CASE( test_sort ){
...
}

BOOST_AUTO_TEST_CASE( test_indirect_stable_sort ){
...
}

@@ -0,0 +1,100 @@
/*
Copyright (c) Marshall Clow 2011-2012.

Choose a reason for hiding this comment

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

Suggested change
Copyright (c) Marshall Clow 2011-2012.
Copyright (c) Marshall Clow 2023.

return ret;
}

/// \fn indirect_partial_sort (RAIterator first, RAIterator last)

Choose a reason for hiding this comment

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

C&P mistake in the signature inside this (and below) docstrings

Comment on lines 155 to 158
/// \fn indirect_nth_element (RAIterator first, RAIterator last, Predicate p)
/// \returns a permutation of the elements in the range [first, last)
/// such that when the permutation is applied to the sequence,
/// the result is sorted according to the predicate pred.

Choose a reason for hiding this comment

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

Similar C&P mistake in signature and description.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch - thanks!

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.

2 participants