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.

Sign up for GitHub

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

Jump to bottom

Simplify Permutations #790

Merged
merged 10 commits into from
Nov 1, 2023
Merged

Simplify Permutations #790

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
bf0564f
Rename `PermutationState` variants
Philippe-Cholet Oct 30, 2023
ff1b1e3
`permutations`: simplify references
Philippe-Cholet Oct 30, 2023
839fa74
`permutations`: `advance` function
Philippe-Cholet Oct 30, 2023
c6de3e2
`PermutationState::size_hint_for`
Philippe-Cholet Oct 30, 2023
8ce6044
Merge `CompleteState` variants into `PermutationState`
Philippe-Cholet Nov 1, 2023
518811a
Unindent block in `Permutations::next`
Philippe-Cholet Nov 1, 2023
901e99b
Inline second inspection of state
Philippe-Cholet Nov 1, 2023
05a815f
Remove `PermutationState::LoadedStart`
Philippe-Cholet Nov 1, 2023
8f0d0ae
`Permutations::next`: simplify two vectors
Philippe-Cholet Nov 1, 2023
8ab3995
`impl FusedIterator for Permutations`
Philippe-Cholet Nov 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
86 changes: 30 additions & 56 deletions src/permutations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Member

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 compute count. Fine given the simplification we gain by this.

Copy link
Member Author

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.

}

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
Copy link
Member

Choose a reason for hiding this comment

The 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 size_hint::map? (We can do this separately, it just occured to me.)

Copy link
Member Author

Choose a reason for hiding this comment

The 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 f for the resulting size hint to be correct, so as I wrote several size hints I went with applying the pattern manually.

(low, upp)
}
}

Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

Could you shortly explain why this debug_assert holds?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 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 for n = vals.size_hint().1.

So in each case: n >= vals.len() >= k. Basically, it holds because we prefilled with k values.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 if n < k { return (0, Some(0)); } (and move the "prefill the lazy buffer" part).

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)),
}
}
}