Skip to content
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.

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

Define metal::accumulate for zero input lists #111

Merged
merged 1 commit into from
Oct 28, 2019
Merged

Conversation

brunocodutra
Copy link
Owner

Speaking of accumulate I decided to have a look at it and realized it could be simplified if implemented in terms of metal::transform.

@brunocodutra
Copy link
Owner Author

@ecrypa would love your feedback on this.

@brunocodutra
Copy link
Owner Author

sigh.

Copy link
Contributor

@ecrypa ecrypa left a comment

Choose a reason for hiding this comment

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

I did see the troubles of old compilers. Anyway, I like the new code.

With Clang 9 and GCC 9 it is also faster than the old accumulate version of min_element.

Comment on lines 57 to 58
#include "../list/at.hpp"
#include "../list/indices.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

In the final version we should check for superfluous includes, right?

Copy link
Owner Author

@brunocodutra brunocodutra Oct 27, 2019

Choose a reason for hiding this comment

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

(Somehow I missed this comment the first time around).

Absolutely, thanks for catching this one!

Comment on lines 68 to 69
template<class state, class vals>
using impl = apply<partial<lbd, state>, vals>;

using type = lambda<impl>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

template<class state, class... seqs>
using type =
forward<_accumulate_impl<state, seqs...>::template type, expr>;
struct _accumulate<state, list<vals...>> {
Copy link
Contributor

@ecrypa ecrypa Oct 17, 2019

Choose a reason for hiding this comment

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

Does this exist for performance reasons? I wonder if we can also increase performance for a total number of two, three, and, say, four seqs. I understand that fold_left plays nicely with a single list, but maybe there is another shortcut if the number of seqs is known.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, by far the most common use case I expect to be accumulating a single list.

I wonder if we can also increase performance for a total number of two, three, and, say, four seqs.

We don't need to, that's the beauty of this version! Notice that it relies on transpose, which already specializes on 1, 2 and 3 lists. That's probably why accumulate got faster in your benchmark.

@brunocodutra
Copy link
Owner Author

With Clang 9 and GCC 9 it is also faster than the old accumulate version of min_element.

Wow nice, didn't expect that to be honest.

I did see the troubles of old compilers.

I hate MSVC with passion.

@brunocodutra brunocodutra force-pushed the accumulate branch 2 times, most recently from 5cba9f1 to 8a14ae0 Compare October 27, 2019 12:41
@brunocodutra brunocodutra changed the title Simplify the implementation of metal::accumulate Define metal::accumulate for zero input lists Oct 27, 2019
@brunocodutra
Copy link
Owner Author

While playing with this I realized metal::accumulate could be generalized to accept zero input lists and decided to change this PR to that instead.

@ecrypa
Copy link
Contributor

ecrypa commented Oct 28, 2019

Beautiful!

Besides, the discarded min_element index retrieval is even faster with this version (v21). See Clang 9 and GCC 9.

@brunocodutra
Copy link
Owner Author

Woah nice, thanks for checking!

@brunocodutra brunocodutra merged commit c17c8ad into master Oct 28, 2019
@brunocodutra brunocodutra deleted the accumulate branch October 28, 2019 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants