Add iterator for combinations

[thofma]

No description provided.

[lgoettgens]

Could you please add some more extensive tests? You can either get inspiration at https://github.com/Nemocas/AbstractAlgebra.jl/pull/2034/files#diff-9383dd485d4500b18b2bfa2797938c914f4a8df63c205e8ea8ddcfe23a1ad538 or test/Combinatorics/EnumerativeCombinatorics/compositions.jl, but I would expect at least the following tests: edge-cases with zero (in particular combinations(0, 0)), something with allunique, testing the guaranteed iteration order (e.g. hardcoding the result of combinations(5,3) which is the smallest example where lex and revlex order differ, and some large example with issorted)

[lgoettgens]

This implementation seems to be slower than AbstractAlgebra.combinations, even when wrapping another collect around it.

julia> @b collect(AbstractAlgebra.combinations(5,3))
615.111 ns (28 allocs: 1.578 KiB)

julia> @b collect(combinations(5,3))
807.188 ns (34 allocs: 1.312 KiB)

julia> @b collect(AbstractAlgebra.combinations(15,10))
250.671 μs (6027 allocs: 547.938 KiB)

julia> @b collect(combinations(15,10))
280.748 μs (9014 allocs: 539.844 KiB)

julia> @b collect(AbstractAlgebra.combinations(18,9))
3.293 ms (97269 allocs: 7.905 MiB)

julia> @b collect(combinations(18,9))
5.897 ms (145865 allocs: 7.790 MiB)

[thofma]

Added more tests and fixed the timings (usual inlining nonsense)

[joschmitt]

Just to say before anything becomes Official API: For the other functions of this kind (partitions, compositions, ...), we have a dedicated type Partition, Composition, ...

[thofma]

I am not quite sure what value that would add here.

[joschmitt]

I am not quite sure what value that would add here.

We inherited the design from JuLie. From what I know, the idea was/is that this is helpful if one wants to work with combinations as objects themselves. So one can then write functions which take a combination as input or something.
I don't know whether there is interest in functionality in this direction, certainly not from my side.

[thofma]

OK, makes sense. I can add that.

[thofma]

Although, then it would not be a drop-in replacement for the current users of AbstractAlgebra.combinations

[lgoettgens]

Just to say before anything becomes Official API: For the other functions of this kind (partitions, compositions, ...), we have a dedicated type Partition, Composition, ...

Although, then it would not be a drop-in replacement for the current users for AbstractAlgebra.combinations

What about having a dedicated return type for calls to combinations(::Int, ::Int), as in these cases people want some combinatorial object. But calls to combinations(::AbstractVector{T}, ::Int) could still return an iterator over Vector{T}.
For the already existing functions of this kind, we only have the former variant, so it would be consistent in this regard. But the latter could still be used as a drop-in-replacement for AbstractAlgebra.combinations.

[fingolfin]

Looks nice

[fingolfin]

Maybe be explicit about returning nothing here, like in line 60?

Suggested change

	state[1] > C.n - C.k + 1 && return
	state[1] > C.n - C.k + 1 && return nothing

[lgoettgens]

Tagging triage to decide about the return type

[mjrodgers]

This looks good, and currently seems to be even a bit faster than the implementation used for OrderedMultiIndexSet.

The idea of having dedicated types is detailed here. Is there a reason it would break compatibility with the AbstractAlgebra version if it's done the same way as Partition? i.e. it is a subtype of AbstractVector and we have a getindex function so we can still access specific entries exactly as if it were a vector.

[fingolfin]

Discussed this in triage. My personal view: let's have this as Oscar.combinations and keep AbstractAlgebra.combinations separate (so that existing code using combinations can be easily adapted to use AbstractAlgebra.combinations for now, and can optionally change to Oscar.combinations later).

Since we don't export combinations from Oscar right now, this would not be breaking for our users.

During triage nobody (dared to) disagree.

[thofma]

OK, I added a Combination type, which is just a thin wrapper around Vector:

julia> collect(combinations(['a', 'b', 'c'], 2))
3-element Vector{Combination{Char}}:
 ['a', 'b']
 ['a', 'c']
 ['b', 'c']

It might provide enough functionality to still be usable in the LieAlgebra module (instead of Vector{Int}). Tests will show.

[thofma]

@lgoettgens seems that the Lie algebra code is still happy with this change. Are you happy with it or should I restore the LieAlgebras.combinations thing?

[JohnAAbbott]

What guarantees does a Combination object make? e.g. its elements are in strictly increasing order?
Can one have a Combination from a multi-set? During triage I asked whether a Combination should also remember what it is a "subset" of; the general opinion seemed to be that it should not store this information.

[thofma]

e.g. its elements are in strictly increasing order?

No, the elements have no ordering. What is ordered are the indices in the lexicographically ascending ordering. (We only consider combinations of Vector).

Can one have a Combination from a multi-set?

No. The signature is combinations(::Vector, ::Int).

During triage I asked whether a Combination should also remember what it is a "subset" of; the general opinion seemed to be that it should not store this information.

Yes, this seems to be the general opinion.

[JohnAAbbott]

No. The signature is combinations(::Vector, ::Int).

Must the vector elements be distinct? If not, what is the behaviour? e.g. if the vector has length n then the combinations are just the vector elements obtained by indexing using the entries of a combination of the integers 1 up to n
Example: collect(combinations([1,1,1],2)) == [Combination([1,1]), Combination([1,1]), Combination([1,1])]

[thofma]

No, they don't have to be distinct and the example you give is the current behaviour. It is consistent with the other functions, e.g.,

julia> subsets([1, 1], 1)
2-element Vector{Vector{Int64}}:
 [1]
 [1]

[lgoettgens]

One usually doesn't want the function checking for uniqueness as that may have a severe performance impact (think e.g. of MPolyIdeals). If this is not the behavior you want to have, you can call unique explicitly beforehand

[lgoettgens]

@lgoettgens seems that the Lie algebra code is still happy with this change. Are you happy with it or should I restore the LieAlgebras.combinations thing?

This was exactly the change the code in LieAlgebras/src/combinatorics.jl was waiting for. I added (mostly naive) implementations there that should be removed/replaced once Oscar has something sensible for this.

Could you also please remove the corresponding testset from experimental/LieAlgebras/test/Combinatorics-test.jl or move them to your new testfile?