Improve merge() algorithm by nordlow · Pull Request #4888 · dlang/phobos

nordlow · 2016-11-04T23:47:39Z

This is revival of my previous #3315 applied on top of existing implementation of merge that sneaked in without me noticing it ;)

Improvements:

Bidirectional support if all inputs are bidirectional
Propagates infiniteness if any of the inputs is infinite.
Add in-place checking of sortedness in popFront and popBack. Done by comparing current and next element if element type allows it.

Other notes:

Checking for isSortedRange in my previous implementation has been removed because we currently cannot compare all the different formats for the comparison alias (pred) fed to SortedRange.

Update:
I just noticed that Merge is used in setops. I guess this removes the opportunity for making this a Voldermort type, right?

andralex · 2016-11-06T12:28:23Z

std/algorithm/sorting.d

-Merge!(less, Rs) merge(alias less = "a < b", Rs...)
-(Rs rs)
+/**
+   Merge multiple sorted ranges $(D rs) with less-than predicate function $(D


for single words prefer backticks to the D macro

andralex · 2016-11-06T12:30:27Z

std/algorithm/sorting.d

+/**
+   Merge multiple sorted ranges $(D rs) with less-than predicate function $(D
+   pred) into one single sorted output range containing the sorted union of the
+   elements of inputs. Elements in the output are not unique, that is the length


s/that is/consequently/

Better yet, rephrase the sentence: "Duplicates are not eliminated, meaning that the total number of elements in the result of merge is the sum of all elements in the ranges passed to it."

andralex · 2016-11-06T12:31:42Z

std/algorithm/sorting.d

+Details:
+
+All of its inputs are assumed to be sorted. This can mean that inputs are
+   instances of $(XREF range, SortedRange). Use the result of $(XREF algorithm,


there is no such restriction in Merge or merge

Ahh, hang on I'll remove this doc.

andralex · 2016-11-06T12:33:41Z

std/algorithm/sorting.d

+   This algorithm is lazy, doing work progressively as elements are pulled off
+   the result.
+
+   Average complexity is $(BIGOH n * k) for $(D k) inputs of maximum length $(D n).


"Time complexity is proportional to the sum of element counts over all ranges."

andralex · 2016-11-06T12:44:16Z

std/algorithm/sorting.d

+   assert(equal(c.retro, [5, 4, 3, 2, 1, 0]));
+   -------
+*/
+Merge!(less, Rs) merge(alias less = "a < b", Rs...)(Rs rs)


The constraints should go here, in the user-visible and documented API, not in the Merge struct. The constraint should not use the package-level CommonElementType.

Done. But we should keep the restriction on Merge because its public and used by setops, right? Later when setUnion is removed we can perhaps turn it into a Voldemort type.

andralex · 2016-11-06T12:44:53Z

std/algorithm/sorting.d

+    assert(m.empty);
+}
+
+package template CommonElementType(Rs...)


this is superfluous

andralex · 2016-11-06T12:48:33Z

You may assert that the ranges are sorted opportunistically; an eager call to isSorted seems a bit much.

nordlow · 2016-11-06T13:46:55Z

So no debug assertions of sortedness of output either, then?

Such a checking can be implemented inside popFront/Back without any extra memory.

nordlow · 2016-11-06T16:03:08Z

Only thing left now is potential checking for sortedness in frontIndex and backIndex.

aG0aep6G · 2016-11-06T16:04:33Z

std/algorithm/sorting.d

+    A range containing the union of the given ranges.
+
+See_Also:
+   $(XREF algorithm, setops, SetUnion)


Please use REF instead of XREF throughout.

aG0aep6G · 2016-11-06T16:16:56Z

std/algorithm/sorting.d

+    A range containing the union of the given ranges.
+
+See_Also:
+   $(REF algorithm, setops, SetUnion)


REF has a different order of arguments than XREF. Symbol name goes first, followed by the module including std. Here: REF(SetUnion, std,algorithm,setops). However, there doesn't seem to be a std.algorithm.setops.SetUnion.

Done. I presume it's because setUnion has been deprecated by merge.

aG0aep6G · 2016-11-06T17:34:34Z

std/algorithm/sorting.d

+    A range containing the union of the given ranges.
+
+See_Also:
+   $(REF setUnion, std,algorithm,setops)


Alright, syntax is correct now. But the link is dead because setUnion has been undocumented. I suggest to just drop the See_Also section.

aG0aep6G · 2016-11-06T17:35:16Z

std/algorithm/sorting.d

+
+All of its inputs are assumed to be sorted. This can mean that inputs are
+   instances of $(REF SortedRange, std,range). Use the result of $(REF sort,
+   std,algorithm), or $(REF assumeSorted, std,range) to merge ranges known to be sorted


The REF to sort is broken. It's std.algorithm.sorting.sort, so the REF must be $(REF sort, std,algorithm,sorting).

aG0aep6G · 2016-11-06T20:02:38Z

Ok, REFs look good to me.

nordlow · 2016-11-06T23:14:12Z

Added debug-only efficient checking for sortedness inside Merge.pop{Front,Back}.

I'm satisfied now.

codecov-io · 2016-11-06T23:39:20Z

Current coverage is 89.39% (diff: 93.40%)

Merging #4888 into master will increase coverage by <.01%

@@             master      #4888   diff @@
==========================================
  Files           124        124          
  Lines         76659      76704    +45   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          68527      68570    +43   
- Misses         8132       8134     +2   
  Partials          0          0

Powered by Codecov. Last update 027839f...e1dfb51

nordlow · 2016-11-10T00:04:18Z

@andralex ping

andralex · 2016-11-10T00:53:15Z

As I mentioned via email I'm on the road this week.

nordlow · 2016-11-10T09:44:52Z

Ok. Thanks, @andralex

Can anyone else approve this?

CyberShadow · 2016-11-15T16:37:35Z

std/algorithm/sorting.d

+   assert(equal(c, [0, 1, 2, 3, 4, 5]));
+   assert(equal(c.retro, [5, 4, 3, 2, 1, 0]));
+   -------
+*/


Unless I'm missing something, the documentation should have a See_Also section linking to setUnion, and an explanation for how this function is different from it.

Ok, I'll add a ref. Notice however that setUnion is not visible online anymore. I find that strange. Is this intentional?

https://dlang.org/phobos/std_algorithm_setops.html

Oh, that's because it's been deprecated in favor of merge. I guess I was missing something after all. Never mind :)

aG0aep6G · 2016-11-21T22:09:37Z

On 11/21/2016 10:53 PM, Per Nordlöw wrote:

These |foreach|es are static ones. I'm pretty sure that the break
associates with |switch| here. If it we're I don't see how the
unittests could pass.

You're mistaken. The break applies to the foreach. The behavior is a bit
weird, though: The break doesn't end the static foreach loop. It jumps
behind the foreach body at run-time, still inside the switch. If there's
code there (e.g. a default case) that gets executed. There may be other
subtle differences.

To break the switch instead, use a label:

sw: switch (foo)
{
     foreach (x; alias_seq)
     {
         case x: /* ... */ break sw;
     }
}

nordlow · 2016-11-21T22:11:29Z

Ok, @aG0aep6G I'll fix.

@aG0aep6G, what do you think about the updated limiting of the sortedness logic?

nordlow · 2016-11-21T22:18:45Z

@aG0aep6G Changed the two switch breaks to be labeled.

nordlow · 2016-11-21T22:22:01Z

And cleaned up the order of the imports and private defs at the top of Merge.

aG0aep6G · 2016-11-21T22:40:19Z

@aG0aep6G, what do you think about the updated limiting of the sortedness logic?

Sorry, I'm not following the actual objective of this PR. Just skimming the messages.

By the way, I think you've edited in that question later on. It's not in the notification email from GItHub. I wouldn't have noticed it if I hadn't had this page open in the browser.

Also, you seem to update the PR by amending. In my experience, that makes it much harder to review the changes. I'd suggest to squash once at the end instead of amending. But Andrei does the actual review here, so that's his call.

nordlow · 2016-11-21T22:40:25Z

Removed isSorted calls in unittest.

nordlow · 2016-11-21T22:42:16Z

Ok, I'll change to not amending and then squashing at the end.

andralex · 2016-11-22T16:20:44Z

std/algorithm/sorting.d

+        static if (isBidirectional)
        {
-            _crt = _crt.max;
+            this._lastBackIndex = size_t.max; // uinitialized, lazy initialized in `front`


Why is this here and not in the initializer of the field?

Removed this assignments because it's initialized to size_t in declaration.

andralex · 2016-11-22T16:22:10Z

std/algorithm/sorting.d

+        Unqual!ElementType bestElement;
+        foreach (i, _; Rs)
+        {
+            if (source[i].empty) { continue; }


This is unusual, for single statements on one line we omit the braces (except when required e.g. for function definitions).

andralex · 2016-11-22T16:24:04Z

std/algorithm/sorting.d

+                                   comp(source[i].front, previousBack),
+                                   "Input " ~ i.stringof ~ " is unsorted"); // @nogc
+                        }
+                    break sw;


as an aside, the label is not necessary - you may just leave the break here and insert one right after the foreach loop

I don't understand. Does that revert back to the first state, where this break associates with static foreach instead of the final switch?

The suggestion is to have two unlabeled breaks, one inside the foreach and another one right after it. But it's really up to you.

andralex · 2016-11-22T16:24:52Z

std/algorithm/sorting.d

        alias opDollar = length;
    }
+
+    public Rs source;


we usually put the state at the beginning

nordlow · 2016-12-16T17:21:44Z

Rebased on master. One thing has changed: improved sortedness checking by breaking out CT-logic into canCheckSortedness.

nordlow · 2016-12-16T20:27:56Z

ci/circleci — Your tests failed on CircleCI fails. Don't know why. Should I repush?

wilzbach

ci/circleci — Your tests failed on CircleCI fails. Don't know why. Should I repush?

Have a look at the output. In the last weeks the CI testing has been improved.
I tried to explain the issues, but in general you can also run the tests locally, e.g. with make -f posix.mak style

wilzbach · 2016-12-17T16:15:01Z

std/algorithm/sorting.d

+   auto c = merge(a.assumeSorted, b.assumeSorted)
+   assert(equal(c, [0, 1, 2, 3, 4, 5]));
+   assert(equal(c.retro, [5, 4, 3, 2, 1, 0]));
+   -------


Any reason why this is a Ddoc unittest block?
If you put it as a normal unittest block and annotate it with the magic /// slashes it will be displayed in the same way, but it has the advantage that we can ensure that the examples does compile. Another advantage is that soon the users will be able to directly run unittests blocks on dlang.org and then of course we will enable this only for tests for which we can ensure they compile & work.

I didn't add this. It's ok to move it to a unittest if ok with the original author.

Ok, I removed it and added a test for bidirectional use (via .retro) of merge result in an existing unittest. Use of assumeSorted isn't revelant anymore.

wilzbach · 2016-12-17T16:15:52Z

std/algorithm/sorting.d

+/// test bi-directional access and common type
+@safe pure nothrow unittest
+{
+    import std.algorithm : equal;


Output of make -f posix.mak style:

grep -nr 'import std.algorithm : ' $(find . -name '*.d') ; test $? -eq 1 ./std/algorithm/sorting.d:1308: import std.algorithm : equal; make: *** [style] Error 1

-> std.algorithm.comparison

wilzbach · 2016-12-17T16:19:11Z

std/algorithm/sorting.d

+
+    auto m = merge(a, b, c);
+
+    static assert(is(typeof(m.front) == CommonType!(S, I, D)));


output of make -f posix.mak publictests:

executing out/std_algorithm_sorting.d out/std_algorithm_sorting.d(165): Error: template instance CommonType!(S, I, D) template 'CommonType' is not defined out/std_algorithm_sorting.d(165): Error: static assert (is(double == _error_)) is false make: *** [publictests] Error 1

As mentioned above we want to ensure that public tests do compile without missing imports and hence we started to automatically check for it. So a public unittest block can't use the global imports.
Solution 1: use the ddoced example as public unittest and make this one private
Solution 2: add the missing import -> import std.traits : CommonType

Added the import for CommonType.

wilzbach · 2016-12-17T16:26:16Z

std/algorithm/sorting.d

-    @property bool empty()
-    {
-        return _crt == _crt.max;
+        size_t bestIndex = size_t.max; // indicate undefined


With ptrdiff_t you could use -1 - this seems to be used at a couple of similar bits at Phobos, e.g.
https://github.com/dlang/phobos/blob/master/std/algorithm/searching.d#L881
https://github.com/dlang/phobos/blob/master/std/string.d#L383

Explanation:
http://en.cppreference.com/w/cpp/types/ptrdiff_t

Why is ptrdiff_t preferred over size_t here? bestIndex is an absolute index here not a difference.

wilzbach

import std.range: retro is still missing :/

wilzbach · 2016-12-19T05:32:53Z

std/algorithm/sorting.d

    int[] b = [2, 3, 4];
+
    assert(a.merge(b).equal([1, 2, 3, 3, 4, 5]));
+    assert(a.merge(b).retro.equal([5, 4, 3, 3, 2, 1]));


make -f publictests or CircleCi

out/std_algorithm_sorting.d(202): Error: no property 'retro' for type 'Merge!("a < b", int[], int[])'

Don't you mean: make -f posix.mak publictests?

Don't you mean: make -f posix.mak publictests?

Yes - sorry for the typo.

wilzbach · 2016-12-19T05:33:19Z

std/algorithm/sorting.d

+    static assert(is(typeof(m.front) == CommonType!(S, I, D)));
+
+    assert(equal(m, [1, 2, 3, 10, 20, 30, 40, 50, 60]));
+    assert(equal(m.retro, [60, 50, 40, 30, 20, 10, 3, 2, 1]));


make -f publictests or CircleCi

out/std_algorithm_sorting.d(169): Error: no property 'retro' for type 'Merge!("a < b", short[], int[], double[])'

wilzbach · 2016-12-19T05:34:08Z

std/algorithm/sorting.d

+
 ///
 @safe pure nothrow unittest
 {


It might be a personal preference, but I show the simpler unittest example as first example to the user.

Done. Moved the longer unittest last.

nordlow · 2016-12-19T09:12:33Z

Ready to pull?

andralex · 2016-12-19T13:42:09Z

std/algorithm/sorting.d

+            }
+            final switch (_lastBackIndex)
+            {
+                foreach (i, _; Rs)


eh, line shown mistakenly as not covered

andralex · 2016-12-19T13:43:56Z

std/algorithm/sorting.d

+    private alias ElementType = CommonType!(staticMap!(.ElementType, Rs));
+    private enum isBidirectional = allSatisfy!(isBidirectionalRange, staticMap!(Unqual, Rs));
+
+    debug private enum canCheckSortedness = isCopyable!ElementType && !hasAliasing!ElementType;


Guess this would be a version:

debug static if (isCopyable!ElementType && !hasAliasing!ElementType) version = can_check_sortedness;

Unsure on which is the best way.

andralex · 2016-12-19T13:45:08Z

Good engineering, thanks.

nordlow force-pushed the better-merge branch 2 times, most recently from 32b2d1a to 060b6fc Compare November 4, 2016 23:58

nordlow changed the title ~~Make merge bi-directional, have infiniteness and more docs~~ Make merge() bi-directional, have infiniteness and more docs Nov 5, 2016

nordlow force-pushed the better-merge branch from 060b6fc to 81c208d Compare November 5, 2016 00:42

nordlow changed the title ~~Make merge() bi-directional, have infiniteness and more docs~~ Improve merge() algorithm Nov 5, 2016

andralex requested changes Nov 6, 2016

View reviewed changes

nordlow force-pushed the better-merge branch 2 times, most recently from b877135 to 5db822d Compare November 6, 2016 16:02

aG0aep6G reviewed Nov 6, 2016

View reviewed changes

nordlow force-pushed the better-merge branch from 5db822d to 82170a5 Compare November 6, 2016 16:09

aG0aep6G reviewed Nov 6, 2016

View reviewed changes

nordlow force-pushed the better-merge branch from 82170a5 to 3bf0b80 Compare November 6, 2016 17:14

aG0aep6G reviewed Nov 6, 2016

View reviewed changes

nordlow force-pushed the better-merge branch 2 times, most recently from c856406 to 25aa2e8 Compare November 6, 2016 19:32

nordlow force-pushed the better-merge branch from 25aa2e8 to e1dfb51 Compare November 6, 2016 23:13

nordlow force-pushed the better-merge branch 2 times, most recently from 16553de to c263e97 Compare November 6, 2016 23:39

aG0aep6G mentioned this pull request Nov 13, 2016

[Ddoc] XREF -> REF #4899

Merged

CyberShadow previously requested changes Nov 15, 2016

View reviewed changes

nordlow force-pushed the better-merge branch from a993e9c to b701476 Compare November 21, 2016 22:08

nordlow force-pushed the better-merge branch from b701476 to 2b2c7aa Compare November 21, 2016 22:18

nordlow force-pushed the better-merge branch from 2b2c7aa to 2aad6cc Compare November 21, 2016 22:21

nordlow force-pushed the better-merge branch from 2aad6cc to 8cf4fcc Compare November 21, 2016 22:40

andralex approved these changes Nov 22, 2016

View reviewed changes

nordlow force-pushed the better-merge branch from 8cf4fcc to 610c32c Compare November 22, 2016 18:20

nordlow force-pushed the better-merge branch 2 times, most recently from 47c762b to 5df9c70 Compare December 16, 2016 17:16

nordlow force-pushed the better-merge branch from 5df9c70 to 8b859b5 Compare December 16, 2016 18:58

wilzbach reviewed Dec 17, 2016

View reviewed changes

nordlow force-pushed the better-merge branch 2 times, most recently from 0b4e8b9 to 3125beb Compare December 18, 2016 12:04

wilzbach reviewed Dec 19, 2016

View reviewed changes

Make merge with bi-directional and have infiniteness and more docs

28153fb

nordlow force-pushed the better-merge branch from 3125beb to 28153fb Compare December 19, 2016 08:11

andralex reviewed Dec 19, 2016

View reviewed changes

std/algorithm/sorting.d

}

final switch (_lastBackIndex)

{

foreach (i, _; Rs)

Copy link

Member

andralex Dec 19, 2016

Choose a reason for hiding this comment

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

eh, line shown mistakenly as not covered

andralex reviewed Dec 19, 2016

View reviewed changes

andralex merged commit cf07aed into dlang:master Dec 19, 2016

nordlow deleted the better-merge branch October 19, 2021 19:45


		auto m = merge(a, b, c);

		static assert(is(typeof(m.front) == CommonType!(S, I, D)));

Uh oh!

Comments

Conversation

nordlow commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nordlow Nov 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex commented Nov 6, 2016

Uh oh!

nordlow commented Nov 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nordlow commented Nov 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nordlow Nov 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aG0aep6G commented Nov 6, 2016

Uh oh!

nordlow commented Nov 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Nov 6, 2016

Current coverage is 89.39% (diff: 93.40%)

Uh oh!

nordlow commented Nov 10, 2016

Uh oh!

andralex commented Nov 10, 2016

Uh oh!

nordlow commented Nov 10, 2016

Uh oh!

CyberShadow Nov 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nordlow Nov 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

nordlow commented Nov 4, 2016 •

edited

Loading

nordlow Nov 6, 2016 •

edited

Loading

nordlow commented Nov 6, 2016 •

edited

Loading

nordlow commented Nov 6, 2016 •

edited

Loading

nordlow Nov 6, 2016 •

edited

Loading

nordlow commented Nov 6, 2016 •

edited

Loading

CyberShadow Nov 15, 2016 •

edited

Loading

nordlow Nov 15, 2016 •

edited

Loading

nordlow commented Nov 21, 2016 •

edited

Loading