Convert partition methods to return iterators

[mjrodgers]

With the recent migration of combinatorics functions from experimental to src in #3159 , there is some work to be done converting the methods to return iterators instead of very large collections of combinatorial objects. This is work to address #3147

Here we are converting the methods that focus on returning integer partitions.

There are 3 different "batches" of partition methods that need to be converted in this way.

1. the basic partitions(n) for computing all integer partitions of n - this has been converted
2. the methods partitions(n,m) (also optionally with upper/lower bounds) for partitioning into a fixed number of parts - this has been converted.
3. the methods partitions(n,v) where v is a vector of allowed values to use in the partition (with options to specify the number of parts, or to restrict the multiplicities of the allowed values) - this one still needs to be converted.

[joschmitt]

Looks good to me so far, but I'm not really an expert on iterators either. Thank you for working on this!

[codecov]

Codecov Report

Attention: Patch coverage is 96.07843% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 81.24%. Comparing base (acc3729) to head (797a968).
Report is 87 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3389      +/-   ##
==========================================
- Coverage   82.80%   81.24%   -1.56%     
==========================================
  Files         577      580       +3     
  Lines       78494    79268     +774     
==========================================
- Hits        64995    64404     -591     
- Misses      13499    14864    +1365     

Files	Coverage Δ
...inatorics/EnumerativeCombinatorics/compositions.jl	95.41% <100.00%> (+1.84%)	⬆️
...rc/Combinatorics/EnumerativeCombinatorics/types.jl	94.87% <100.00%> (+2.87%)	⬆️
...rics/EnumerativeCombinatorics/weak_compositions.jl	93.44% <100.00%> (+1.25%)	⬆️
...mbinatorics/EnumerativeCombinatorics/partitions.jl	96.54% <95.69%> (-1.08%)	⬇️

... and 160 files with indirect coverage changes

Outdated

[lkastner]

Another question I have: You seem to rely on this being fast. Are there some timing tests to make sure it stays fast?

[joschmitt]

Another question I have: You seem to rely on this being fast. Are there some timing tests to make sure it stays fast?

This is an ancient issue #697...

Also, in this case the comparison is not completely fair. From my experience with the other combinatoric's things, calling collect on the iterator is a bit slower than just generating the whole array in one go. Without being the target audience for these functions, I would argue that one usually doesn't call collect, though, and the iterator is much more versatile.

[lkastner]

Another question I have: You seem to rely on this being fast. Are there some timing tests to make sure it stays fast?

This is an ancient issue #697...

Also, in this case the comparison is not completely fair. From my experience with the other combinatoric's things, calling collect on the iterator is a bit slower than just generating the whole array in one go. Without being the target audience for these functions, I would argue that one usually doesn't call collect, though, and the iterator is much more versatile.

I didn't mean to compare to the older version, I simply wanted to recommend adding some performance tests now. They could just be for the iterator without collect. Nevertheless, I'll leave that decision to you.

[fingolfin]

@lkastner how do you envision performance tests would work? I.e. who would run them to test for performance regressions? We can't do this in CI, as timings fluctuate wildly.

[lkastner]

@lkastner how do you envision performance tests would work? I.e. who would run them to test for performance regressions? We can't do this in CI, as timings fluctuate wildly.

But we already do run timing tests in CI? https://github.com/oscar-system/Oscar.jl/blob/master/test/PolyhedralGeometry/timing.jl

[lgoettgens]

I think #3389 (comment) is the only unresolved comment above.

[joschmitt]

I think #3389 (comment) is the only unresolved comment above.

I'll assume that one can replace the partitions method form intersection theory with one of the established ones. But this is unrelated to this pull request; I will open a new one. Let's merge this.