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

Implement traditional group-by as found in most other languages like clojure/scala/ruby/etc #39

Merged
merged 2 commits into from Jan 11, 2014

Conversation

sbinq
Copy link
Contributor

@sbinq sbinq commented Jan 11, 2014

This default group-by logic is different from what is called groupby within itertools.

@aavezel
Copy link

aavezel commented Jan 11, 2014

from itertools import groupby
lst = ['1', '12', 'a', '123', 'ab']
keyfunc = lambda x: len(x)
for k, g in groupby(sorted(lst, key=keyfunc), key=keyfunc):
    print k
    print list(g)

@kachayev
Copy link
Owner

@aavezel The main problem with given solution is that we need to calculate keyfunc(item) twice of each item in lst (it can be problematic if keyfunc is something more sophisticated than len).

@sbinq
Copy link
Contributor Author

sbinq commented Jan 11, 2014

@aavezel @kachayev yes, agree with @kachayev arguments against itertools.groupby. Also don't like the idea of having to write such boilerplate (well, not much - but still a little annoying) and convert manually to dict.

@aavezel
Copy link

aavezel commented Jan 11, 2014

@kachayev : you are mistake. itertools.groupby return iterators, and not calculate keys if it not needed. It is more suitable for principle of lazy calculation.

@kachayev
Copy link
Owner

@aavezel

  1. sorted function returns a list, not iterable. So there is no room for lazy calculation.
  2. In your code keyfunc will be evaluated for each element by sorted function (cause it's necessary to compare items on sorting) and then it be evaluated by groupby function to return you k value and find out the boundaries of each group.

@ghost ghost assigned kachayev Jan 11, 2014
@sbinq
Copy link
Contributor Author

sbinq commented Jan 11, 2014

@kachayev updated code according to your review - styling issues and more explicit test

kachayev added a commit that referenced this pull request Jan 11, 2014
Implement traditional group-by as found in most other languages like clojure/scala/ruby/etc, thanks to @sbinq
@kachayev kachayev merged commit 40d5550 into kachayev:master Jan 11, 2014
@kachayev
Copy link
Owner

@sbinq Thanks for your contribution!

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.

None yet

3 participants