rust-itertools / itertools

Extra iterator adaptors, iterator methods, free functions, and macros.
https://docs.rs/itertools/
Apache License 2.0
2.72k stars 309 forks source link

`Powerset` count #735

Closed Philippe-Cholet closed 1 year ago

Philippe-Cholet commented 1 year ago

@phimuemue After counting combinations last week, I thought a simple PR would be nice to start the week.

The first three commits are about Powerset::count while the next four commits are about Powerset::size_hint.

Philippe-Cholet commented 1 year ago

Powerset::count needs n = pool.count() and self.combs.count() which also needs n. Without filling the lazy buffer, we can not compute the real n twice. That's why Combinations::n_and_count exists, to be able to re-use n.

I replaced Powerset::size_hint implementation (and removed the pos field since it is its only purpose) because it removes the fallback (when self.pos overflows, which seems unlikely to me but who knows). Then I was unsure what to do with size_hint::pow_scalar_base: remove it or allow it to be dead code? I temporarily chose the second. Feel free to cherry-pick the first three commits if you don't like my size_hint changes.

Philippe-Cholet commented 1 year ago

Tomorrow, I'll add tests with (0..5).filter(|i| i % 2 == 0).chain(5..10).combinations(k)/powerset() which will have nice size hints. I also thought it would be interesting to test. Otherwise, I think I answered all points.

Philippe-Cholet commented 1 year ago

I can do a similar test for powerset if you want but I'm gonna wait for your feedback first.

phimuemue commented 1 year ago

Thank you a lot!

bors r+

As for the new tests: I’m not sure if they too closely reimplement the internal logic and assert that the reimplementation matches the original.

I will have a closer look later. I think what I’d test is that the size hints do not become only more exact over time and that they are consistent with count.

bors[bot] commented 1 year ago

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here. For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.