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

combinations: count and size_hint #729

Merged
merged 14 commits into from
Aug 18, 2023

Conversation

Philippe-Cholet
Copy link
Member

The indices we track (to give combinations in the lexicographic order) can help us count the remaining combinations.

@phimuemue That's all for combinations. Each commit is as small as possible. Similar work for other iterators to come, but this should be the biggest as it prepares work for powerset and combinations_with_replacement.

Because we give combinations in the lexicographic order, we can use the indices to count the remaining combinations to come.
We are gonna heavily test it thought.
The whole point of the lazy buffer is to not fill the pool all at once but in some cases we want to, and then it's better than do it with `while lazy_buffer.get_next() {}` that would be weird.
Copy link
Member

@phimuemue phimuemue left a comment

Choose a reason for hiding this comment

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

Hi @Philippe-Cholet I tried to review, and I have some questions.

I think the most difficult part for me is to understand remaining_for.

src/lazy_buffer.rs Show resolved Hide resolved
src/combinations.rs Outdated Show resolved Hide resolved
src/combinations.rs Outdated Show resolved Hide resolved
src/combinations.rs Outdated Show resolved Hide resolved
src/combinations.rs Outdated Show resolved Hide resolved
src/combinations.rs Outdated Show resolved Hide resolved
`pool.it.count()` consumes `pool` before we call `remaining_for` so it can not be a method and therefore becomes a free function.
Copy link
Member

@phimuemue phimuemue left a comment

Choose a reason for hiding this comment

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

Thanks for the update. We should comment the sources for the complex algorithms and add some tests, then I think we're good to go.

src/combinations.rs Outdated Show resolved Hide resolved
src/combinations.rs Show resolved Hide resolved
src/combinations.rs Outdated Show resolved Hide resolved
src/combinations.rs Show resolved Hide resolved
src/combinations.rs Outdated Show resolved Hide resolved
tests/test_std.rs Show resolved Hide resolved
tests/test_std.rs Outdated Show resolved Hide resolved
tests/test_std.rs Outdated Show resolved Hide resolved
tests/test_std.rs Outdated Show resolved Hide resolved
src/combinations.rs Show resolved Hide resolved
@Philippe-Cholet
Copy link
Member Author

Philippe-Cholet commented Aug 18, 2023

All changes are done (with a limit of 500 for test_checked_binomial).
And I added a TODO about "sum of options" (possible for a MSRV of 1.37, currently 1.36) that would really make remaining_for more readable. (It also pass the test obviously.)

Ready to squash everything into one commit.
And the next PR will be easier because of the changes done here.

@phimuemue phimuemue merged commit 25c86b8 into rust-itertools:master Aug 18, 2023
@Philippe-Cholet Philippe-Cholet deleted the count_combinations branch September 6, 2023 17:26
@jswrenn jswrenn mentioned this pull request Nov 14, 2023
@jswrenn jswrenn added this to the next milestone Nov 14, 2023
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.

3 participants