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

Add partitions function #276

Merged
merged 1 commit into from
Mar 25, 2019
Merged

Add partitions function #276

merged 1 commit into from
Mar 25, 2019

Conversation

rominf
Copy link
Contributor

@rominf rominf commented Mar 23, 2019

Add partitions function

Partitioning set or interval is a known thing in mathematics. This PR adds a function to do find all partitions of a collection. For more information what is considered a partition, see docstring.

Thanks, Tigran Saluev (http://saluev.com/) for the implementation.

UPD: the code was rewritten from scratch.

@bbayles
Copy link
Collaborator

bbayles commented Mar 23, 2019

Thanks for the PR.

A few comments:

  • It looks like this relies on slicing. Could you make it such that a generic iterable could be used?
  • flake8 is complaining about a long line (see Travis output)
  • Could we get Tigran Saluev's blessing on including his implementation?

Thanks again.

@Saluev
Copy link

Saluev commented Mar 23, 2019

👍

@Saluev
Copy link

Saluev commented Mar 23, 2019

In my code though the first line was collection = list(collection), so the problem 1 did not exist.

@rominf
Copy link
Contributor Author

rominf commented Mar 23, 2019

It looks like this relies on slicing. Could you make it such that a generic iterable could be used?

It's possible, but then you will get a list of lists of strings with length 1 if the input is the string. To make them strings user will have to write a quite long and slow code. Also, making list(iterable) will implicitly consume generators which is a bad thing and consume memory. I think that if the user wants to pass a generator, he has to explicitly (explicit is better than implicit, as Python Zen states) call list, which is not hard at all:

partitions(list(generator))

@Saluev
Copy link

Saluev commented Mar 23, 2019

Just in case: I don't agree with that. I think consuming a generator passed to an iteration tool is totally fine. It always happens anyway. itertools.product and other alike functions HAVE to do that. And I don't think we should do strings-driven optimizations in an itertools library as well.

(Also I consider the reference to Zen to be pure demagogy.)

@rominf
Copy link
Contributor Author

rominf commented Mar 23, 2019

Now the iterables are supported, but collections are left as is, without converting them to lists.
@bbayles, is everything OK?

@rominf
Copy link
Contributor Author

rominf commented Mar 23, 2019

I rewrote the code from scratch. Now it's shorter and works 30% faster.

more_itertools/more.py Outdated Show resolved Hide resolved
@rominf
Copy link
Contributor Author

rominf commented Mar 24, 2019

Now function converts all iterables to the lists.

@bbayles, @pylang is everything OK?

Partitioning set or interval is a known thing in mathematics. This
commit adds a function to do find all partitions of a collection. For
more information what is considered a partition, see docstring.
@bbayles bbayles merged commit 6e7f3ec into more-itertools:master Mar 25, 2019
@bbayles
Copy link
Collaborator

bbayles commented Mar 25, 2019

Merged with some additional tests and docstring changes. This will be in the next release. Many thanks!

@rominf rominf deleted the partitions branch March 25, 2019 03:57
@rominf
Copy link
Contributor Author

rominf commented Mar 25, 2019

@bbayles, I think that the difference between partition and this function should be noted.

partition divides iterable into two, but those iterable are not contiguous in the original iterable (function takes some items from iterable, then skips some items, then takes, and so on). But partitions generates all contiguous itrerables.

I've tried to describe it in a formal way. You've removed that. OK, but then, insert, please, an informal note about this.

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.

4 participants