Skip to content

Conversation

@mjrodgers
Copy link
Collaborator

@mjrodgers mjrodgers commented Nov 7, 2025

For use cases that benefit, we provide an option to have inplace versions of the iterators for combinations(n,k) and multicombinations(n,k) using an optional inplace keyword.

Closes #4993

@mjrodgers mjrodgers added the release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes label Nov 7, 2025
@mjrodgers mjrodgers marked this pull request as ready for review November 7, 2025 10:36
@fingolfin fingolfin added enhancement New feature or request topic: combinatorics labels Nov 7, 2025
@fingolfin fingolfin changed the title Inplace iterators (optional) for Combinations/Multicombinations Support optional "inplace" iteration for Combinations/Multicombinations Nov 7, 2025
@lgoettgens
Copy link
Member

So if I understand correctly, the "inplace" kwarg only exists for the case of combinations(::Int, ::Int), and not combinations(::Vector, ::Int), right?
Does it make sense to also allow it in the former case? (Even it does, maybe do that in a future PR).

Could you add the new kwarg to the two respective docstrings?

@mjrodgers
Copy link
Collaborator Author

mjrodgers commented Nov 7, 2025

So if I understand correctly, the "inplace" kwarg only exists for the case of combinations(::Int, ::Int), and not combinations(::Vector, ::Int), right? Does it make sense to also allow it in the former case? (Even it does, maybe do that in a future PR).

Could you add the new kwarg to the two respective docstrings?

I don't believe it makes sense, or at the very least would not be possible without more extensive changes; I would have to think about how to accomplish this, we don't get it for "free" from the state in this case.

Do we want the kwarg in the docstrings? WeakCompositions (which is the only existing EnumerativeCombinatorics iterator that already used this) doesn't have one, I thought maybe this was by design.

@lgoettgens
Copy link
Member

So if I understand correctly, the "inplace" kwarg only exists for the case of combinations(::Int, ::Int), and not combinations(::Vector, ::Int), right? Does it make sense to also allow it in the former case? (Even it does, maybe do that in a future PR).
Could you add the new kwarg to the two respective docstrings?

I don't believe it makes sense, or at the very least would not be possible without more extensive changes; I would have to think about how to accomplish this, we don't get it for "free" from the state in this case.

Do we want the kwarg in the docstrings? WeakCompositions (which is the only existing EnumerativeCombinatorics iterator that already used this) doesn't have one, I thought maybe this was by design.

Two good points, then better leave it like it is (at least for now).

But could you please add a test that makes use of the new kwarg?

@mjrodgers
Copy link
Collaborator Author

mjrodgers commented Nov 11, 2025

As long as all the tests still pass after my most recent change, I think this is ready to merge; I will open an issue to bring up the discussion of Int vs BigInt for the length methods applied to combinatorial structures, and a separate pull request to add details about multicombinations / number_of_multicombinations to the Oscar documentation (since that requires some changes that are somewhat orthogonal to the rest of this pull request).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request 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.

Improve performance of Combinations ~~for the special case of 99% of the usage?~~ with inplace option

3 participants