Change History (18)

Maybe I should add that I'm interested, of course, in *iterating* through all those partitions. Just running "RestrictedPartitions(3000, [1000, 500, 100, 50, 10])" is very fast because it just returns an iterator. The actual code I'm running is stuff like

For ease of reviewing, I plan to have two patches here: the first one includes deprecation for RestrictedPartitions? and fixes up some problems with the Partitions docstring. The second will actually implement the new Partitions(..., parts_in=...) code.

Summary
changed from RestrictedPartitions is very slow and a huge memory hog to [with patch, needs review] RestrictedPartitions is very slow and a huge memory hog

The second patch gets doctests working and adds the new functionality. I'm not entirely sure I did the deprecation stuff correctly; I added a deprecation warning, and then fiddled with all the doctests in the old RestrictedPartitions? code so that it passes doctests.

A little ego update to the patch -- I added myself to the authors list. Anyone reviewing this might want to look at my test script: ​restricted_partitions_test_suite.sage. I've run over 10,000 successful tests with it.

Summary
changed from [with patch, needs review] RestrictedPartitions is very slow and a huge memory hog to [with patch, needs minor work+rebase] RestrictedPartitions is very slow and a huge memory hog

Patch look good. However, before finishing my review I'm waiting it to be rebased on top of 3.4.1. Also some interface problem have been discussed by e-mail: It was decided more that one month ago that a combinatorial class should define:

This script is great. I should be put in sage in one place or one other. If someone tries to optimize your code (eg: Cythonize), he surely will be happy to have your test code. Why not shipping it into the patch with a more explicit name and a one-line example on how to use it with the comment " #longtest " ?

If starting=p is passed, then the combinatorial class of partitions greater than or equal to p in lexicographic order is returned.

is clearer if phrased

If starting=p is passed, then the combinatorial class of partitions with part greater than or equal to p in lexicographic order is returned.

"p" refers to a partition, not to a part, so the text is okay. Perhaps we should make that more clear, though. I plan on opening a ticket to improve the documentation in partition.py, so we can do that then.

Anyone reviewing this might want to look at my test script: restricted_partitions_test_suite.sage. I've run over 10,000 successful tests with it.

This script is great. I should be put in sage in one place or one other. If someone tries to
optimize your code (eg: Cythonize), he surely will be happy to have your test code. Why not
shipping it into the patch with a more explicit name and a one-line example on how to use it
with the comment " #longtest " ?

Well, right now, the script only halts if it finds an error, so it would be a really, really long test. :)

The tests also can use lots of memory, since it puts a list of the partitions into memory. But if anyone thinks (a mildly modified version of) the script should get run on a "long" test, I'm happy to have it included.

Summary
changed from [with patch, needs review] RestrictedPartitions is very slow and a huge memory hog to [with patch, positive review] RestrictedPartitions is very slow and a huge memory hog

The patch looks good. The doctests passed except that depending on the base the doctests numbers may differ. So I added a review patch which replace the explicit doctest numbers with .... This should solve this problem.