Skip to content

Commit f5dc73f

Browse files
committed
Stop quick argsort from also running a stable sort
The quick case in detail::argsort_iter had no break, so it fell through to the stable case. Every argsort with sorting_method::quick, the default, ran std::sort and then std::stable_sort on the same indices. The result was still a valid ordering, but the work roughly doubled. Add the missing break statements. The new test counts the comparisons made by the quick path and checks them against a plain std::sort on the same data. On master it fails with 108 comparisons against 70.
1 parent 57f3d07 commit f5dc73f

2 files changed

Lines changed: 44 additions & 0 deletions

File tree

‎include/xtensor/misc/xsort.hpp‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -318,6 +318,7 @@ namespace xt
318318
return comp(*(data_begin + i), *(data_begin + j));
319319
}
320320
);
321+
break;
321322
}
322323
case (sorting_method::stable):
323324
{
@@ -329,6 +330,7 @@ namespace xt
329330
return comp(*(data_begin + i), *(data_begin + j));
330331
}
331332
);
333+
break;
332334
}
333335
}
334336
}

‎test/test_xsort.cpp‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@
77
* The full license is in the file LICENSE, distributed with this software. *
88
****************************************************************************/
99

10+
#include <algorithm>
11+
#include <numeric>
12+
#include <vector>
13+
1014
#include "xtensor/containers/xadapt.hpp"
1115
#include "xtensor/containers/xarray.hpp"
1216
#include "xtensor/containers/xfixed.hpp"
@@ -176,6 +180,44 @@ namespace xt
176180
EXPECT_EQ(ex, xt::argsort(a, {0}, xt::sorting_method::stable));
177181
}
178182

183+
TEST(xsort, argsort_quick_sorts_once)
184+
{
185+
// The quick method must not fall through to the stable sort: both
186+
// orderings are valid, so only the number of comparisons tells them apart.
187+
const std::vector<double> data = {5., 3., 8., 1., 9., 2., 7., 4., 6., 0., 3., 8., 1., 5., 2.};
188+
189+
std::size_t argsort_comparisons = 0;
190+
std::vector<std::size_t> indices(data.size());
191+
detail::argsort_iter(
192+
data.cbegin(),
193+
data.cend(),
194+
indices.begin(),
195+
indices.end(),
196+
[&argsort_comparisons](double lhs, double rhs)
197+
{
198+
++argsort_comparisons;
199+
return lhs < rhs;
200+
},
201+
sorting_method::quick
202+
);
203+
204+
std::size_t sort_comparisons = 0;
205+
std::vector<std::size_t> expected(data.size());
206+
std::iota(expected.begin(), expected.end(), std::size_t(0));
207+
std::sort(
208+
expected.begin(),
209+
expected.end(),
210+
[&](std::size_t i, std::size_t j)
211+
{
212+
++sort_comparisons;
213+
return data[i] < data[j];
214+
}
215+
);
216+
217+
EXPECT_EQ(indices, expected);
218+
EXPECT_EQ(argsort_comparisons, sort_comparisons);
219+
}
220+
179221
TEST(xsort, sort_easy)
180222
{
181223
xarray<double> a = {{5, 3, 1}, {4, 4, 4}};

0 commit comments

Comments
 (0)