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

Reducers #1102

Merged
merged 6 commits into from May 23, 2013
Merged

Reducers #1102

merged 6 commits into from May 23, 2013

Conversation

ericmj
Copy link
Member

@ericmj ericmj commented May 22, 2013

No description provided.

Remove Enum.equal?
Remove File.iterator/2 and File.biniterator/2
@@ -117,16 +119,14 @@ defmodule Enum do
"""
@spec count(t, (element -> as_boolean(term))) :: non_neg_integer
def count(collection, fun) when is_list(collection) do
do_count(collection, fun)
folder = fn(x, acc) -> if fun.(x), do: acc+1, else: acc end
:lists.foldl(folder, 0, collection)
Copy link
Member

Choose a reason for hiding this comment

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

Since both this clause and the next clause are basically doing a reduction, I think we can get rid of this one altogether.

do_drop_while(list, fun)
end
I.reduce(collection, [], fn(entry, acc) ->
if fun.(entry), do: [], else: [entry|acc]
Copy link
Member

Choose a reason for hiding this comment

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

This one is wrong. Basically, after the first time the function returns false, we get the rest of the list. It should be something like:

fn
  (entry, []) -> if fun.(entry), do: [], else: [entry]
  (entry, acc) -> [entry|acc]
end

We are basically using the accumulator to check if something was added so far (or not).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, will add tests for it aswell.

@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

list when is_list(list) ->
do_all?(list, fun)
end
I.reduce(collection, nil, fn(entry, _) ->
Copy link
Member

Choose a reason for hiding this comment

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

Replace nil by true here and in the next line, and we can get rid of the true return value before catch.

@josevalim
Copy link
Member

I have seen that you kept the specific clauses for lists in many functions but we can get rid of many of them. We definitely should remove: Enum.count/2, Enum.join/2, Enum.map_join/3, Enum.partition/2, Enum.map_reduce/3, Enum.reduce/2 as the reducer clause will provide the same performance benefits.

We should also rewrite Enum.max(list), Enum.min(list) to use reducers instead of :lists.max(list) and :lists.min(list) (I want to avoid dependency on :lists which is not tail recursive).

We should consider removing the specific lists clauses for Enum.all?/2, Enum.any?/2, Enum.fetch/2, Enum.find_index/2, Enum.find/3 and Enum.find_value/3 too. The only extra overhead is a throw. But let's not worry about it now, we can always discuss those later.

@ericmj
Copy link
Member Author

ericmj commented May 22, 2013

@josevalim New commit with discussed changes.

@josevalim
Copy link
Member

There are two things pending I believe:

  1. Should we still call the protocol Enum.Iterator? We could rename it to Enumerable, Enum.Reducible, Enum.Collection, etc
  2. Docs for reduce in the protocol

/cc @alco @devinus @yrashk @ericmj please tell me your thoughts

@ericmj
Copy link
Member Author

ericmj commented May 22, 2013

I think I prefer Enum.Reducible. I'm working on the docs.

@meh
Copy link
Contributor

meh commented May 22, 2013

I'd call it Enumerable, the point of it is clearer, Enum.Reducible doesn't sound clear at all.

@ericmj
Copy link
Member Author

ericmj commented May 22, 2013

I'd call it Enumerable, the point of it is clearer, Enum.Reducible doesn't sound clear at all.

It is simply something that is reducible with the reduce function. Although, we also supply the functions member? and count so Reducible might not be the perfect name.

@alco
Copy link
Member

alco commented May 22, 2013

I'd like to see how the docs will describe this new stuff before deciding on the name.

@ericmj
Copy link
Member Author

ericmj commented May 22, 2013

Added documentation

@alco
Copy link
Member

alco commented May 22, 2013

It would be nice to have some historical background of the reasons for migrating from iterator to reduce. Not necessarily in the docs, it could be mentioned in a commit message.

Since the main point of the protocol is reducing a collection, I'd call it Enum.Reducer. A collection is called reducible if it implements Reducer, i.e. it is compatible with Enum functions.

Enumerable doesn't quite fit -- it implies enumeration of a collection. But Enum doesn't simply enumerate, it produces something new (a list) for the given collection. Therefore, Reducer is sufficiently descriptive in my view -- we'd call a reducer something that helps transform the collection by means of the reduce function.

@josevalim
Copy link
Member

The problem with calling it reducer is that, if we add another important operation to Enum.Reducer, let's say map, should we still call it a reducer?

@meh
Copy link
Contributor

meh commented May 22, 2013

Yeah, I think it makes sense to call it Enumerable because that's what it does is make it work with Enum functions, it's as general as possible, even considering most functions just use reduce, it's better to be safe and make it general than be sorry later and be stuck with a deadly breakage or unfit name.

@josevalim
Copy link
Member

@alco I am writing a blog post about it. That will be included in the release announcement. :)

josevalim pushed a commit that referenced this pull request May 23, 2013
@josevalim josevalim merged commit 66476b3 into elixir-lang:master May 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants