Add std.algorithm.iteration.cumulativeFold by dcarp · Pull Request #3972 · dlang/phobos

dcarp · 2016-02-04T23:59:45Z

This PR implements std.algorithm.iteration.scan.

http://forum.dlang.org/post/n8gini$1ibc$1@digitalmars.com
http://en.cppreference.com/w/cpp/algorithm/partial_sum

quickfur · 2016-02-05T00:06:31Z

This seems to be a rather useful function, but scan is a horrible name for it. My first reaction to seeing the name was, "isn't this already implemented as find, findUntil, or each?". We need a better name that reflects what the function does.

dcarp · 2016-02-05T00:16:51Z

Actually I find the name quite good. It is already used in haskell [1] and C++ [2] seams to adopt it also.
[1] https://www.haskell.org/hoogle/?hoogle=scanl
[2] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4310.html#parallel.alg.inclusive.scan

andralex · 2016-02-07T22:38:26Z

Thanks for the contribution. I don't have bandwidth for it just now, but I'll note that the name does not do it justice. I'd already forgotten what the algorithm does (something with partial sums) and the name definitely didn't remind me of anything, maybe except... FoxPro :o).

dcarp · 2016-02-07T23:59:31Z

On wikipedia it is called "prefix sum", "scan" or "cumulative sum". Considering the generic implementation, I think that the name that does not contain the word "sum" is the better one :).
Anyhow, if some good proposals are presented, I'm happy to change the name.

quickfur · 2016-02-10T17:53:52Z

What about partials?

JakobOvrum · 2016-02-10T18:15:04Z

std/algorithm/iteration.d

+    assert(sum2.array == [1, 3, 6, 10, 15]);
+
+    // Compute the maximum of all elements
+    auto largest = scan!(max)(arr);


Examples should use single-token template argument list instantiation shortcut: scan!max(arr)

edit:

Oh, this isn't actually an example unit test... was that intentional? It's full of the kind of redundancies I'd expect from an example and not a traditional unit test.

JakobOvrum · 2016-02-10T18:23:35Z

The implementation should propagate forward range capability and the tests should test more kinds of ranges than just slices; see std.internal.test.dummyrange for helpers.

JakobOvrum · 2016-02-10T18:25:31Z

changelog.dd


 $(BUGSTITLE Library Changes,

+$(LI $(LNAME2 std-algorithm-iteration-scan, $(XREF algorithm.iterator, scan)


XREF doesn't work like this. Please use $(REF scan, std, algorithm, iteration)

JakobOvrum · 2016-02-10T18:35:14Z

I think the documentation would be better if the overloads were consolidated in one place with /// Ditto like we've been encouraging as of late. In particular it allows eliminating redundant information across the board, improving the signal to noise ratio.

dcarp · 2016-02-10T23:13:15Z

@quickfur: considering that the function is called on each iteration, I think that a "verb" name is more appropriate. Moreover partials can be easily confused with std.functional.partial.

ntrel · 2016-02-29T13:29:40Z

I suggest we call this something with fold in the name (assuming we're committed to renaming reduce to fold). It's the same calculation as fold, just packaged differently. Maybe lazyFold, cumulativeFold, progressiveFold, successiveFold.

wilzbach · 2016-02-29T13:59:52Z

I vote for cumulativeFold - imho the best name for this.

quickfur · 2016-02-29T15:37:10Z

@greenify I like that name. Thanks!

schuetzm · 2016-02-29T19:26:15Z

stepwiseFold?

quickfur · 2016-02-29T20:17:30Z

That works too.

dcarp · 2016-02-29T21:22:14Z

partialFold would work too...
At the moment I made the changes for cumulativeFold. If there are no strong opinions against it, I propose to go with it.

wilzbach · 2016-03-04T10:43:47Z

std/algorithm/iteration.d

+        else
+            alias State = staticMap!(ReduceSeedType!(ElementType!R), binfuns);
+
+        foreach (i, f; binfuns)


@andralex said once on a review of one of my PRs that foreach shouldn't be used for generic code.

I'm not sure that I agree that foreach shouldn't be used in generic code. Sometimes, it's exactly what's appropriate. It's just that there's often a better way to do it that doesn't involve foreach, and you have to be careful with the element type, since foreach will infer strings to have their code unit as their element type and not dchar.

Regardless, in this case, it looks like this is a static foreach (binfuns is the result of staticMap), so it pretty much has to be done either with a foreach or a recursive template. The foreach is likely to be cleaner and more efficient, particularly when this is inside of a function rather than something that's part of the public API.

This is static, there's no other way.

dcarp · 2016-03-09T16:06:58Z

@quickfur: is cumulativeFold name telling enough? Other open points?

DmitryOlshansky · 2016-03-11T15:35:08Z

cumulativeFold sounds about right to me

dcarp · 2016-03-28T16:27:54Z

@andralex are you ok with cumulativeFold? Other proposed names were: stepwiseFold, partialFold...

wilzbach · 2016-03-30T20:05:50Z

Needs the @andralex tag for name review!

wilzbach · 2016-04-08T08:39:02Z

Ping @andralex

wilzbach · 2016-04-25T19:50:09Z

I just accidentally ended up implementing this myself, because I forgot about the PR and obviously there were no docs yet :/
-> +1 for moving forward with this PR.

Btw in Scala this is called scanLeft.
It would be great to get the "name ok" from @andralex for cumulativeFold.

dcarp · 2016-04-25T20:44:53Z

Actually the original name of this function was scan (suggested by Timon Gehr), but this was more or less rejected as unrecognizable.
It is quite annoying that every week or so, I need to rebase this PR because of the change log entry and slowly I'm thinking about giving up on this :(.

wilzbach · 2016-04-25T20:46:41Z

It is quite annoying that every week or so, I need to rebase this PR because of the change log entry and slowly I'm thinking about giving up on this :(.

Please don't, there's hope:
#4228

ntrel · 2016-04-26T09:36:05Z

partialFold would work too...

I prefer that, it's like STL's partial_sum but more general.

andralex · 2016-04-26T13:59:13Z

std/algorithm/iteration.d

+it returns the first element unchanged and uses it as seed for the next
+elements.
+This function is also known as `partial_sum`, `accumulate`, `prefix_sum`,
+`scan`, or `cumulative_sum`.


Making some or all of these names hot links would be great.

andralex · 2016-04-26T14:06:39Z

Thanks for this solid piece of work. I approve the addition, please pull once the nits are looked at.

dcarp · 2016-04-27T11:35:03Z

Thank you for the review! All comments were addressed.

wilzbach · 2016-04-29T09:08:28Z

I approve the addition, please pull once the nits are looked at.
Thank you for the review! All comments were addressed.

So we are good to go here? :)

DmitryOlshansky · 2016-04-29T09:14:56Z

Auto-merge toggled on

JakobOvrum reviewed Feb 10, 2016
View reviewed changes

dcarp changed the title ~~Add std.algorithm.iteration.scan~~ Add std.algorithm.iteration.cumulativeFold Mar 3, 2016

wilzbach reviewed Mar 4, 2016
View reviewed changes

dcarp mentioned this pull request Mar 19, 2016

Fix issue 15807 #4091

Merged

schveiguy added the @andralex label Mar 31, 2016

andralex reviewed Apr 26, 2016
View reviewed changes

dcarp added 2 commits April 27, 2016 22:12

Add std.algorithm.iteration.cumulativeFold

5a9cedf

Address review comments

984fbb4

DmitryOlshansky merged commit 85f9e81 into dlang:master Apr 29, 2016

dcarp deleted the std_algorithm_iteration_scan branch April 29, 2016 17:51

wilzbach mentioned this pull request Jun 25, 2016

Add std.random.discrete: Discrete sampler libmir/mir#234

Merged

e-y-e mentioned this pull request Oct 29, 2016

Add new function std.algorithm.iteration : cumulativeSum #4881

Merged


		$(BUGSTITLE Library Changes,

		$(LI $(LNAME2 std-algorithm-iteration-scan, $(XREF algorithm.iterator, scan)

Uh oh!

Comments

Conversation

dcarp commented Feb 4, 2016

Uh oh!

quickfur commented Feb 5, 2016

Uh oh!

dcarp commented Feb 5, 2016

Uh oh!

andralex commented Feb 7, 2016

Uh oh!

dcarp commented Feb 7, 2016

Uh oh!

quickfur commented Feb 10, 2016

Uh oh!

JakobOvrum Feb 10, 2016

Choose a reason for hiding this comment

Uh oh!

JakobOvrum commented Feb 10, 2016

Uh oh!

JakobOvrum Feb 10, 2016

Choose a reason for hiding this comment

Uh oh!

JakobOvrum commented Feb 10, 2016

Uh oh!

dcarp commented Feb 10, 2016

Uh oh!

ntrel commented Feb 29, 2016

Uh oh!

wilzbach commented Feb 29, 2016

Uh oh!

quickfur commented Feb 29, 2016

Uh oh!

schuetzm commented Feb 29, 2016

Uh oh!

quickfur commented Feb 29, 2016

Uh oh!

dcarp commented Feb 29, 2016

Uh oh!

wilzbach Mar 4, 2016

Choose a reason for hiding this comment

Uh oh!

jmdavis Mar 4, 2016

Choose a reason for hiding this comment

Uh oh!

andralex Apr 26, 2016

Choose a reason for hiding this comment

Uh oh!

dcarp commented Mar 9, 2016

Uh oh!

DmitryOlshansky commented Mar 11, 2016

Uh oh!

dcarp commented Mar 28, 2016

Uh oh!

wilzbach commented Mar 30, 2016

Uh oh!

wilzbach commented Apr 8, 2016

Uh oh!

wilzbach commented Apr 25, 2016

Uh oh!

dcarp commented Apr 25, 2016

Uh oh!

wilzbach commented Apr 25, 2016

Uh oh!

ntrel commented Apr 26, 2016

Uh oh!

andralex Apr 26, 2016

Choose a reason for hiding this comment

Uh oh!

andralex commented Apr 26, 2016

Uh oh!

dcarp commented Apr 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilzbach commented Apr 29, 2016

Uh oh!

DmitryOlshansky commented Apr 29, 2016

Uh oh!

Reviewers

Assignees

dcarp commented Apr 27, 2016 •

edited

Loading