-
Notifications
You must be signed in to change notification settings - Fork 315
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
Simplify Permutations
#790
Changes from 1 commit
bf0564f
ff1b1e3
839fa74
c6de3e2
8ce6044
518811a
901e99b
05a815f
8f0d0ae
8ab3995
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -135,54 +135,16 @@ where | |
} | ||
|
||
fn count(self) -> usize { | ||
fn from_complete(complete_state: CompleteState) -> usize { | ||
complete_state | ||
.remaining() | ||
.expect("Iterator count greater than usize::MAX") | ||
} | ||
|
||
let Permutations { vals, state } = self; | ||
match state { | ||
PermutationState::Start { k } => { | ||
let n = vals.count(); | ||
let complete_state = CompleteState::Start { n, k }; | ||
|
||
from_complete(complete_state) | ||
} | ||
PermutationState::Buffered { k, min_n } => { | ||
let prev_iteration_count = min_n - k + 1; | ||
let n = vals.count(); | ||
let complete_state = CompleteState::Start { n, k }; | ||
|
||
from_complete(complete_state) - prev_iteration_count | ||
} | ||
PermutationState::Loaded(state) => from_complete(state), | ||
PermutationState::End => 0, | ||
} | ||
let Self { vals, state } = self; | ||
let n = vals.count(); | ||
state.size_hint_for(n).1.unwrap() | ||
} | ||
|
||
fn size_hint(&self) -> SizeHint { | ||
let at_start = |k| { | ||
// At the beginning, there are `n!/(n-k)!` items to come (see `remaining`) but `n` might be unknown. | ||
let (mut low, mut upp) = self.vals.size_hint(); | ||
low = CompleteState::Start { n: low, k } | ||
.remaining() | ||
.unwrap_or(usize::MAX); | ||
upp = upp.and_then(|n| CompleteState::Start { n, k }.remaining()); | ||
(low, upp) | ||
}; | ||
match self.state { | ||
PermutationState::Start { k } => at_start(k), | ||
PermutationState::Buffered { k, min_n } => { | ||
// Same as `Start` minus the previously generated items. | ||
size_hint::sub_scalar(at_start(k), min_n - k + 1) | ||
} | ||
PermutationState::Loaded(ref state) => match state.remaining() { | ||
Some(count) => (count, Some(count)), | ||
None => (::std::usize::MAX, None), | ||
}, | ||
PermutationState::End => (0, Some(0)), | ||
} | ||
let (mut low, mut upp) = self.vals.size_hint(); | ||
low = self.state.size_hint_for(low).0; | ||
upp = upp.and_then(|n| self.state.size_hint_for(n).1); | ||
Comment on lines
+129
to
+130
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This pattern seems familiar to me... Do you know if it occurs somewhere else? If so, should we introduce There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
(low, upp) | ||
} | ||
} | ||
|
||
|
@@ -222,22 +184,34 @@ impl CompleteState { | |
} | ||
} | ||
} | ||
} | ||
|
||
/// Returns the count of remaining permutations, or None if it would overflow. | ||
fn remaining(&self) -> Option<usize> { | ||
match self { | ||
&CompleteState::Start { n, k } => { | ||
if n < k { | ||
return Some(0); | ||
} | ||
(n - k + 1..=n).try_fold(1usize, |acc, i| acc.checked_mul(i)) | ||
impl PermutationState { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you shortly explain why this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
So in each case: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. However, I'm considering to soon work on making all our iterators lazy (such as #602) and I'll surely turn that assertion into |
||
let total = (n - k + 1..=n).try_fold(1usize, |acc, i| acc.checked_mul(i)); | ||
(total.unwrap_or(usize::MAX), total) | ||
}; | ||
match *self { | ||
Self::Start { k } => at_start(n, k), | ||
Self::Buffered { k, min_n } => { | ||
// Same as `Start` minus the previously generated items. | ||
size_hint::sub_scalar(at_start(n, k), min_n - k + 1) | ||
} | ||
CompleteState::Ongoing { indices, cycles } => { | ||
cycles.iter().enumerate().try_fold(0usize, |acc, (i, &c)| { | ||
Self::Loaded(CompleteState::Start { n, k }) => at_start(n, k), | ||
Self::Loaded(CompleteState::Ongoing { | ||
ref indices, | ||
ref cycles, | ||
}) => { | ||
let count = cycles.iter().enumerate().try_fold(0usize, |acc, (i, &c)| { | ||
acc.checked_mul(indices.len() - i) | ||
.and_then(|count| count.checked_add(c)) | ||
}) | ||
}); | ||
(count.unwrap_or(usize::MAX), count) | ||
} | ||
Self::End => (0, Some(0)), | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, we go via
SizeHint
(i.e.(usize, Option<usize>)
) to computecount
. Fine given the simplification we gain by this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.