JuliaMath / Combinatorics.jl

A combinatorics library for Julia
http://juliamath.github.io/Combinatorics.jl/dev/
Other
215 stars 58 forks source link

Fix integer partitions #82

Open jessebett opened 5 years ago

jessebett commented 5 years ago

This would close #79 #80 and #81.

Solution is due to @simonschoelly.

Note that integer_partitions is still implemented, but now just collects partitions(n::Int). However, this implementation now returns the partitions in reverse order as before, so this would be a breaking change! Probably best to just remove integer_partitions

Also, partitions(0) now correctly returns the set containing the empty set and is the same as what's returned by integer_partitions(0), which was not the case before.

codecov-io commented 5 years ago

Codecov Report

Merging #82 into master will increase coverage by 18.72%. The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #82       +/-   ##
===========================================
+ Coverage   75.51%   94.23%   +18.72%     
===========================================
  Files           7        7               
  Lines         637      503      -134     
===========================================
- Hits          481      474        -7     
+ Misses        156       29      -127
Impacted Files Coverage Δ
src/partitions.jl 97.43% <100%> (+21.72%) :arrow_up:
src/numbers.jl 100% <0%> (+14.66%) :arrow_up:
src/combinations.jl 80.64% <0%> (+15.98%) :arrow_up:
src/multinomials.jl 88.88% <0%> (+16.16%) :arrow_up:
src/permutations.jl 97.64% <0%> (+17.06%) :arrow_up:
src/youngdiagrams.jl 93.93% <0%> (+19.24%) :arrow_up:
src/factorials.jl 100% <0%> (+23.07%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 38fc5c4...0d5f9c7. Read the comment docs.

mschauer commented 5 years ago

@jessebett

mschauer commented 4 years ago

What do we do with this? Can we make this non-breaking?

jessebett commented 4 years ago

@mschauer sorry this was lost in my github notifications. To answer your question, I don't know how to make it non-breaking. Not sure why integer_partitions was ever implemented in the first place, let alone to have a completely different behaviour than partitions(n::Int) (referring to the reversed order, not the iterator vs array).

So, seems to me this should be a breaking change to remove the integer_partitions. I am not familiar with the preferred procedure for this, but we could give it a deprecation notice and also have it call collect(reverse(partitions(n::Int))) in the back-end?

mschauer commented 4 years ago

I would opt for deprecate it and keep it doing what it did before instead of collecting partitions(n::Int)

jessebett commented 4 years ago

Agreed.