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

Fix integer partitions #82

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jessebett
Copy link

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
Copy link

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%) ⬆️
src/numbers.jl 100% <0%> (+14.66%) ⬆️
src/combinations.jl 80.64% <0%> (+15.98%) ⬆️
src/multinomials.jl 88.88% <0%> (+16.16%) ⬆️
src/permutations.jl 97.64% <0%> (+17.06%) ⬆️
src/youngdiagrams.jl 93.93% <0%> (+19.24%) ⬆️
src/factorials.jl 100% <0%> (+23.07%) ⬆️

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.

function Base.iterate(p::IntegerPartitions, xs = Int[])
function Base.iterate(p::IntegerPartitions)
p.n == 0 && return (Int[], Int[])
return iterate(p, Int[])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return iterate(p, Int[])
return iterate(p, Int[])

Indentation

@@ -17,7 +17,12 @@ end
Base.length(p::IntegerPartitions) = npartitions(p.n)
Base.eltype(p::IntegerPartitions) = Vector{Int}

function Base.iterate(p::IntegerPartitions, xs = Int[])
function Base.iterate(p::IntegerPartitions)
p.n == 0 && return (Int[], Int[])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
p.n == 0 && return (Int[], Int[])
p.n == 0 && return (Int[], Int[])

@mschauer
Copy link
Member

@jessebett

@mschauer
Copy link
Member

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

@jessebett
Copy link
Author

@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
Copy link
Member

mschauer commented Dec 5, 2019

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

@jessebett
Copy link
Author

Agreed.

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.

partitions(0) does not return empty set
3 participants