Skip to content

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


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

@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

3 participants