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

implement Iterator::nth for Combinations #329

Closed
wants to merge 1 commit into from

Conversation

fyrchik
Copy link
Contributor

@fyrchik fyrchik commented Feb 2, 2019

Hello!
I have implemented nth method for Combinations iterator.

Closes #301 .
Let me know, what do you think. Other changes in tests are due to rustfmt.

src/combinations.rs Outdated Show resolved Hide resolved
@jswrenn jswrenn self-assigned this Jul 18, 2019
@fyrchik
Copy link
Contributor Author

fyrchik commented Jul 30, 2019

I have rebased this on master.

@jswrenn
Copy link
Member

jswrenn commented Jul 31, 2019

Thanks! I'll take a look at this soon.

@jswrenn
Copy link
Member

jswrenn commented Aug 2, 2019

@fyrchik I've filed a PR adding a quickcheck test on your feature branch: fyrchik#1

That test case is finding differences between the output of your specialized Combinations::nth and the current unspecialized method.

The specialized version of Combinations::nth needs to behave identically to the current unspecialized method before I can merge this PR.

@fyrchik
Copy link
Contributor Author

fyrchik commented Aug 5, 2019

I have reimplemented this and got rid of binomial coefficients.
We are still advancing iterator one-by-one, however there are no intermediate allocations.
I have also added quickcheck tests from @jswrenn to be sure that new implementations behaves similar to the old.

@jswrenn
Copy link
Member

jswrenn commented Jan 20, 2020

Thanks! Could you resolve the merge conflict? Also, note that the behavior of combinations(0) changed in #383.

@fyrchik
Copy link
Contributor Author

fyrchik commented Feb 6, 2020

I have rebased on master. cargo +nightly test finished successfully.

@Philippe-Cholet
Copy link
Member

Philippe-Cholet commented Feb 7, 2024

Hi @fyrchik, Combinations has evolved a bit since you worked on this.
Do you want to update your work here? I would promptly respond/review.
We now have tests about specializations so I don't think we need your tests.

EDIT: Well, I see all your repositories are archived, I'm closing this.
I'll probably make this myself when I find more time unless someone does this before me.

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

Successfully merging this pull request may close these issues.

Faster skipping combinations with nth in Combinations
4 participants