Skip to content

Add std.algorithm.iteration.cumulativeFold #3972

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

Jump to bottom
Merged
merged 2 commits into from
Apr 29, 2016
Merged

Add std.algorithm.iteration.cumulativeFold #3972

merged 2 commits into from
Apr 29, 2016

Conversation

dcarp
Copy link
Contributor

@dcarp dcarp commented Feb 4, 2016

@quickfur
Copy link
Member

quickfur commented Feb 5, 2016

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
Copy link
Contributor Author

dcarp commented Feb 5, 2016

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

@dcarp dcarp force-pushed the std_algorithm_iteration_scan branch 2 times, most recently from 5eac01f to a38bcd8 Compare February 5, 2016 00:37
@andralex
Copy link
Member

andralex commented Feb 7, 2016

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
Copy link
Contributor Author

dcarp commented Feb 7, 2016

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.

@dcarp dcarp force-pushed the std_algorithm_iteration_scan branch from 312f221 to 19ff3cb Compare February 9, 2016 23:34
@quickfur
Copy link
Member

What about partials?

JakobOvrum
JakobOvrum reviewed Feb 10, 2016
View reviewed changes
std/algorithm/iteration.d
assert(sum2.array == [1, 3, 6, 10, 15]);

// Compute the maximum of all elements
auto largest = scan!(max)(arr);
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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
JakobOvrum reviewed Feb 10, 2016
View reviewed changes
changelog.dd

)

$(BUGSTITLE Library Changes,

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

Choose a reason for hiding this comment

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

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

@JakobOvrum
Copy link
Contributor

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 dcarp force-pushed the std_algorithm_iteration_scan branch from f205f96 to 942aecb Compare February 10, 2016 22:28
@dcarp
Copy link
Contributor Author

dcarp commented Feb 10, 2016

@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.

@dcarp dcarp force-pushed the std_algorithm_iteration_scan branch 2 times, most recently from 5cc297a to 883d093 Compare February 22, 2016 09:30
@ntrel
Copy link
Contributor

ntrel commented Feb 29, 2016

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
Copy link
Contributor

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

@quickfur
Copy link
Member

@greenify I like that name. Thanks!

@schuetzm
Copy link
Contributor

stepwiseFold?

@dcarp dcarp force-pushed the std_algorithm_iteration_scan branch 2 times, most recently from 67ca912 to b1c564a Compare February 29, 2016 19:41
@quickfur
Copy link
Member

That works too.

@dcarp
Copy link
Contributor Author

dcarp commented Feb 29, 2016

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.

@dcarp dcarp changed the title Add std.algorithm.iteration.scan Add std.algorithm.iteration.cumulativeFold Mar 3, 2016
@dcarp dcarp force-pushed the std_algorithm_iteration_scan branch from 72dcb3c to d6ca72e Compare March 3, 2016 21:45
@dcarp dcarp force-pushed the std_algorithm_iteration_scan branch from 146dfe3 to 453ea69 Compare March 28, 2016 16:24
@dcarp
Copy link
Contributor Author

dcarp commented Mar 28, 2016

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

@wilzbach
Copy link
Contributor

Needs the @andralex tag for name review!

@dcarp dcarp force-pushed the std_algorithm_iteration_scan branch from 453ea69 to dd2f60c Compare April 7, 2016 23:53
@wilzbach
Copy link
Contributor

wilzbach commented Apr 8, 2016

Ping @andralex

@dcarp dcarp force-pushed the std_algorithm_iteration_scan branch from dd2f60c to 35e6a83 Compare April 11, 2016 18:33
@dcarp dcarp force-pushed the std_algorithm_iteration_scan branch from 35e6a83 to 1030e87 Compare April 20, 2016 21:57
@wilzbach
Copy link
Contributor

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
Copy link
Contributor Author

dcarp commented Apr 25, 2016

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
Copy link
Contributor

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
Copy link
Contributor

ntrel commented Apr 26, 2016

partialFold would work too...

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

andralex
andralex reviewed Apr 26, 2016
View reviewed changes
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`.
Copy link
Member

Choose a reason for hiding this comment

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

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

@andralex
Copy link
Member

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

@dcarp dcarp force-pushed the std_algorithm_iteration_scan branch from 1030e87 to 74fbbac Compare April 27, 2016 03:55
@dcarp
Copy link
Contributor Author

dcarp commented Apr 27, 2016 •
edited
Loading

Thank you for the review! All comments were addressed.

@dcarp dcarp force-pushed the std_algorithm_iteration_scan branch from 74fbbac to 984fbb4 Compare April 27, 2016 20:13
@wilzbach
Copy link
Contributor

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
Copy link
Member

Auto-merge toggled on

@DmitryOlshansky DmitryOlshansky merged commit 85f9e81 into dlang:master Apr 29, 2016
@dcarp dcarp deleted the std_algorithm_iteration_scan branch April 29, 2016 17:51
@wilzbach wilzbach mentioned this pull request Jun 25, 2016
Merged
@e-y-e e-y-e mentioned this pull request Oct 29, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@dcarp @quickfur @andralex @JakobOvrum @ntrel @wilzbach @schuetzm @DmitryOlshansky @jmdavis @schveiguy