Skip to content

Fix issue 15421 - The behaviours of the topNs differ with the bottom #3865

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.

Sign up for GitHub

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

Closed
Infiltrator wants to merge 2 commits into from
Closed

Fix issue 15421 - The behaviours of the topNs differ with the bottom #3865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aae9f46
Improve BinaryHeap's handling of ranges
Infiltrator Dec 10, 2015
d524a9d
Update topN(R, R) to match topN(R, i) behaviour
Infiltrator Dec 8, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 50 additions & 2 deletions std/algorithm/sorting.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2294,9 +2294,13 @@ void topN(alias less = "a < b",
static assert(ss == SwapStrategy.unstable,
"Stable topN not yet implemented");
auto heap = BinaryHeap!Range1(r1);
for (; !r2.empty; r2.popFront())
foreach (ref e; r2)
{
heap.conditionalInsert(r2.front);
auto f = heap.front;
if (heap.conditionalInsert(e))
{
e = f;
}
}
}

Expand All @@ -2310,6 +2314,50 @@ unittest
assert(a == [0, 1, 2, 2, 3]);
}

unittest
{
import std.algorithm.comparison : equal;
import std.internal.test.dummyrange;
import std.meta : AliasSeq;
alias RandomRanges = AliasSeq!(
DummyRange!(ReturnBy.Reference, Length.Yes, RangeType.Random),
DummyRange!(ReturnBy.Value, Length.Yes, RangeType.Random),
);

foreach (T1; RandomRanges)
{
foreach (T2; AllDummyRanges)
{
T1 a;
T2 b;
topN(a, b);
sort(a);
assert(equal(a, [ 1, 1, 2, 2, 3, 3, 4, 4, 5, 5 ]));
sort(b);
assert(equal(b, [ 6, 6, 7, 7, 8, 8, 9, 9, 10, 10 ]));
}
}
}

// bug 15421
unittest
{
auto a = [ 9, 8, 0, 3, 5, 25, 43, 4, 2, 0, 7 ];
auto b = [ 9, 8, 0, 3, 5, 25, 43, 4, 2, 0, 7 ];

topN(a, 4);
topN(b[0 .. 4], b[4 .. $]);

sort(a[0 .. 4]);
sort(a[4 .. $]);
sort(b[0 .. 4]);
sort(b[4 .. $]);

assert(a[0 .. 4] == b[0 .. 4]);
assert(a[4 .. $] == b[4 .. $]);
assert(a == b);
}

/**
Copies the top $(D n) elements of the input range $(D source) into the
random-access range $(D target), where $(D n =
Expand Down
71 changes: 62 additions & 9 deletions std/container/binaryheap.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,26 @@ if (isRandomAccessRange!(Store) || isRandomAccessRange!(typeof(Store.init[])))
{
assert(!store.empty, "Cannot pop an empty store.");
if (store.length == 1) return;
auto t1 = moveFront(store[]);
auto t2 = moveBack(store[]);
static if(isRandomAccessRange!Store)
{
auto t1 = moveFront(store);
Copy link
Contributor

@JakobOvrum JakobOvrum Dec 10, 2015

Choose a reason for hiding this comment

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

Perhaps instead of using store everywhere, add an internal primitive like:

static if (isRandomAccessRange!Store)
    alias range = store;
else
    auto range() @property
    {
        return store[];
    }

Then use range everywhere. However, BinaryHeap clearly needs more tests too, and at this point, it would probably be better to fix it in a separate PR. It also has a strange thing going on where it uses the package(std) symbol HeapOps from std.algorithm.sorting.

Copy link
Contributor Author

@Infiltrator Infiltrator Dec 13, 2015

Choose a reason for hiding this comment

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

Yeah, I was thinking something like that. But then there's functions like pop(Store store). I guess that I could just leave that one be.

auto t2 = moveBack(store);
}
else
{
auto t1 = moveFront(store[]);
auto t2 = moveBack(store[]);
}
store.front = move(t2);
store.back = move(t1);
percolate(store[], 0, store.length - 1);
static if(isRandomAccessRange!Store)
{
percolate(store, 0, store.length - 1);
}
else
{
percolate(store[], 0, store.length - 1);
}
}

public:
Expand Down Expand Up @@ -147,7 +162,10 @@ the heap work incorrectly.
_store = move(s);
_length = min(_store.length, initialSize);
if (_length < 2) return;
buildHeap(s[]);
static if (isRandomAccessRange!Store)
buildHeap(s);
else
buildHeap(s[]);
assertValid();
}

Expand Down Expand Up @@ -293,13 +311,28 @@ Removes the largest element from the heap.
enforce(!empty, "Cannot call removeFront on an empty heap.");
if (_length > 1)
{
auto t1 = moveFront(_store[]);
auto t2 = moveAt(_store[], _length - 1);
static if(isRandomAccessRange!Store)
{
auto t1 = moveFront(_store);
auto t2 = moveAt(_store, _length - 1);
}
else
{
auto t1 = moveFront(_store[]);
auto t2 = moveAt(_store[], _length - 1);
}
_store.front = move(t2);
_store[_length - 1] = move(t1);
}
--_length;
percolate(_store[], 0, _length);
static if(isRandomAccessRange!Store)
{
percolate(_store, 0, _length);
}
else
{
percolate(_store[], 0, _length);
}
}

/// ditto
Expand All @@ -325,7 +358,14 @@ Replaces the largest element in the store with $(D value).
// must replace the top
assert(!empty, "Cannot call replaceFront on an empty heap.");
_store.front = value;
percolate(_store[], 0, _length);
static if(isRandomAccessRange!Store)
{
percolate(_store, 0, _length);
}
else
{
percolate(_store[], 0, _length);
}
debug(BinaryHeap) assertValid();
}

Expand All @@ -349,7 +389,10 @@ must be collected.
assert(!_store.empty, "Cannot replace front of an empty heap.");
if (!comp(value, _store.front)) return false; // value >= largest
_store.front = value;
percolate(_store[], 0, _length);
static if (isRandomAccessRange!Store)
percolate(_store, 0, _length);
else
percolate(_store[], 0, _length);
debug(BinaryHeap) assertValid();
return true;
}
Expand Down Expand Up @@ -427,3 +470,13 @@ unittest
static assert(isInputRange!(typeof(h)));
assert(h.equal([16, 14, 10, 9, 8, 7, 4, 3, 2, 1]));
}

unittest
{
import std.internal.test.dummytest;
foreach(DummyRange; AllDummyRanges)
{
auto a = [4, 1, 3, 2, 16, 9, 10, 14, 8, 7];
auto h = heapify(a);
}
}