Skip to content

Import Multipartition functionality from JuLie#4746

Merged
fingolfin merged 48 commits intooscar-system:masterfrom
mjrodgers:Multipartition
Apr 9, 2025
Merged

Import Multipartition functionality from JuLie#4746
fingolfin merged 48 commits intooscar-system:masterfrom
mjrodgers:Multipartition

Conversation

@mjrodgers
Copy link
Copy Markdown
Collaborator

Import Multipartition functionality from JuLie.
Fixes #4656

Comment thread src/Combinatorics/EnumerativeCombinatorics/types.jl Outdated
Comment thread src/Combinatorics/EnumerativeCombinatorics/types.jl Outdated
Comment thread test/Combinatorics/EnumerativeCombinatorics/multipartitions.jl Outdated
Comment thread src/Combinatorics/EnumerativeCombinatorics/multipartitions.jl Outdated
Comment thread src/Combinatorics/EnumerativeCombinatorics/multipartitions.jl
Copy link
Copy Markdown
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Thank you for getting this started! Now we need to clean this up some more

Comment thread src/Combinatorics/EnumerativeCombinatorics/multipartitions.jl Outdated
Comment thread src/Combinatorics/EnumerativeCombinatorics/multipartitions.jl Outdated
Comment thread src/Combinatorics/EnumerativeCombinatorics/multipartitions.jl Outdated
Comment thread src/Combinatorics/EnumerativeCombinatorics/multipartitions.jl Outdated
Comment thread src/Combinatorics/EnumerativeCombinatorics/multipartitions.jl Outdated
Comment thread src/Combinatorics/EnumerativeCombinatorics/multipartitions.jl Outdated
Comment thread src/Combinatorics/EnumerativeCombinatorics/multipartitions.jl Outdated
Comment thread src/Combinatorics/EnumerativeCombinatorics/multipartitions.jl Outdated
Comment thread src/Combinatorics/EnumerativeCombinatorics/multipartitions.jl Outdated
Comment thread test/Combinatorics/EnumerativeCombinatorics/multipartitions.jl Outdated
@mjrodgers
Copy link
Copy Markdown
Collaborator Author

Couple of spare things to clean up, but this should hopefully pass tests now and be ready for more serious review.

@mjrodgers mjrodgers marked this pull request as ready for review March 26, 2025 16:21
@thofma
Copy link
Copy Markdown
Collaborator

thofma commented Mar 31, 2025

I vaguely remember that we used artificial iterators to make the API "stable" (even if we don't have a proper iterator yet). Maybe @joschmitt remembers?

@joschmitt
Copy link
Copy Markdown
Member

I vaguely remember that we used artificial iterators to make the API "stable" (even if we don't have a proper iterator yet). Maybe @joschmitt remembers?

Yes, before the OSCAR 1.0 release, I turned all the return some_array into return (x for x in some_array).

@mjrodgers
Copy link
Copy Markdown
Collaborator Author

mjrodgers commented Mar 31, 2025 via email

Comment thread docs/src/Combinatorics/EnumerativeCombinatorics/multipartitions.md
Partition{Int64}[[], [2]]

julia> collect(multipartitions(2,2))
5-element Vector{Multipartition{Int64}}:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this perhaps like this?

Suggested change
5-element Vector{Multipartition{Int64}}:
5-element Vector{Partition{Int64}}:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But CI test pass... It seems I am misunderstanding something... huh

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I agree that it seems like this should be Vector{Multipartition}, this definitely passes CI but that doesn't mean that it's what we want... I'm looking at it now.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ah okay, I understand the confusion. multipartitions returns ALL the multipartitions of n into r parts, so we get a Vector{Multipartition}. While each individual Multipartion looks like a Vector{Partition}. So this is all correct.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ahah I get it now:

julia> foo = collect(MP)
5-element Vector{Multipartition{Int64}}:
 Partition{Int64}[[], [2]]
 Partition{Int64}[[], [1, 1]]
 Partition{Int64}[[1], [1]]
 Partition{Int64}[[2], []]
 Partition{Int64}[[1, 1], []]

julia> typeof(foo)
Vector{Multipartition{Int64}} (alias for Array{Multipartition{Int64}, 1})

julia> foo[1]
Partition{Int64}[[], [2]]

julia> typeof(foo[1])
Multipartition{Int64}

So my confusion is that Multipartition is printed as a Vector{Partition}. Which makes some sense, but at least for me it would be easier to understand what is going on if there was a custom show` method...

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I can do this but I'm not sure what it should look like. We have the same issue with Partition printing as Vector{Int}:

julia> P = partition(6, 4, 4, 2)
[6, 4, 4, 2]

Comment thread src/Combinatorics/EnumerativeCombinatorics/multipartitions.jl Outdated
@mjrodgers
Copy link
Copy Markdown
Collaborator Author

If I compare to what is detailed about the Combinatorics API, I should maybe have a dedicated Multipartitions type that is a wrapper around the iterator returned by multipartitions, with a dedicated show method. I think this should be pretty straightforward.

@fingolfin
Copy link
Copy Markdown
Member

@mjrodgers are you working on that then?

Comment thread docs/doc.main Outdated
Co-authored-by: Max Horn <max@quendi.de>
@mjrodgers
Copy link
Copy Markdown
Collaborator Author

@mjrodgers are you working on that then?

After our last conversation, I thought we decided to merge this and work on the printing in a separate pull request; but I can take care of this before merging if you think that's better.

@fingolfin
Copy link
Copy Markdown
Member

@mjrodgers ok I forgot about that, I am happy to do it like you said :-)

@fingolfin fingolfin merged commit a688a68 into oscar-system:master Apr 9, 2025
30 of 32 checks passed
fieker pushed a commit that referenced this pull request May 16, 2025
Co-authored-by: Max Horn <max@quendi.de>
Co-authored-by: Lars Göttgens <lars.goettgens@gmail.com>
@lgoettgens lgoettgens added the release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes label May 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: combinatorics

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Integration of multipartition combinatorics to Oscar

5 participants