Simplify Permutations
#790
Conversation
And small documentation of the variants.
There was a problem hiding this comment.
Hi @Philippe-Cholet, looks good to me. The flattened structure seems much nicer.
This took me a while to review, and I could only do it by "replaying" what you possibly did yourself (see d188e8e...phimuemue:rust-itertools:permutations-states-2, the end result being exactly what you proposed). Unfortunately, I am not aware of a process that would allow quicker reviews apart from more fine-grained commits.
Maybe it's worth preserving the history of fine-grained transformations. Please use your judgment, and either merge this or take my commits and merge them.
| } | ||
| let Self { vals, state } = self; | ||
| let n = vals.count(); | ||
| state.size_hint_for(n).1.unwrap() |
There was a problem hiding this comment.
So, we go via SizeHint (i.e. (usize, Option<usize>)) to compute count. Fine given the simplification we gain by this.
There was a problem hiding this comment.
Indeed, size_hint_for mostly ends with (x.unwrap_or(usize::MAX), x) on which we either do .0 or .1.
At worst, it needlessly unwrapped an option one time.
| fn size_hint_for(&self, n: usize) -> SizeHint { | ||
| // At the beginning, there are `n!/(n-k)!` items to come. | ||
| let at_start = |n, k| { | ||
| debug_assert!(n >= k); |
There was a problem hiding this comment.
Could you shortly explain why this debug_assert holds?
There was a problem hiding this comment.
I wrote that in my previous branch some weeks ago so I was not much familiar with it, and take a glance at it is definitely not enough as it requires a bit of thinking.
This debug_assert! only occurs with Start and Buffered variants.
At definition, we prefill the lazy buffer with k values. It has enough values (or we would have the End variant) so vals.len() >= k (vals.len()==k at definition, more later).
size_hint_for is then called with:
- in the case of
count:n = vals.count() >= vals.len()(see lazy buffer for>=) ; - in the case of
size_hint:n = vals.size_hint().0 >= vals.len()(see lazy buffer).
Similar forn = vals.size_hint().1.
So in each case: n >= vals.len() >= k. Basically, it holds because we prefilled with k values.
There was a problem hiding this comment.
However, I'm considering to soon work on making all our iterators lazy (such as #602) and I'll surely turn that assertion into if n < k { return (0, Some(0)); } (and move the "prefill the lazy buffer" part).
| low = self.state.size_hint_for(low).0; | ||
| upp = upp.and_then(|n| self.state.size_hint_for(n).1); |
There was a problem hiding this comment.
This pattern seems familiar to me... Do you know if it occurs somewhere else? If so, should we introduce size_hint::map? (We can do this separately, it just occured to me.)
There was a problem hiding this comment.
This pattern occurs most of the time I think. I thought of a similar method in a messy PR but I had headaches about the conditions on f for the resulting size hint to be correct, so as I wrote several size hints I went with applying the pattern manually.
Philippe-Cholet
force-pushed
the
permutations-states-2
branch
from
4f049ca to
8ab3995
Compare
|
@phimuemue You can check that there is no difference between our branches: |
No worries. I hope it didn't come across an offense. Thanks again for taking the time to simplify this. |
|
@phimuemue I did not see any offense, only the opportunity to improve the way I write commits. |
Fixes #747
This is quite a rewrite, very little is untouched.
countandsize_hintare really simplified with the newPermutationState::size_hint_for.I did not expect any performance improvement, but nice to have a little one.