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

Added array_permutations (attempt 2) #1014

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/lazy_buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,15 @@ pub trait ArrayOrVecHelper: BorrowMut<[usize]> {
fn item_from_fn<T, F: Fn(usize) -> T>(len: Self::Length, f: F) -> Self::Item<T>;

fn len(&self) -> Self::Length;

fn from_fn<F: Fn(usize) -> usize>(k: Self::Length, f: F) -> Self;

fn start(len: Self::Length) -> Self
where
Self: Sized,
{
Self::from_fn(len, |i| i)
}
}

impl ArrayOrVecHelper for Vec<usize> {
Expand All @@ -133,6 +142,10 @@ impl ArrayOrVecHelper for Vec<usize> {
fn len(&self) -> Self::Length {
self.len()
}

fn from_fn<F: Fn(usize) -> usize>(k: Self::Length, f: F) -> Self {
(0..k).map(f).collect()
}
}

impl<const K: usize> ArrayOrVecHelper for [usize; K] {
Expand All @@ -153,4 +166,8 @@ impl<const K: usize> ArrayOrVecHelper for [usize; K] {
fn len(&self) -> Self::Length {
ConstUsize::<K>
}

fn from_fn<F: Fn(usize) -> usize>(_len: Self::Length, f: F) -> Self {
std::array::from_fn(f)
}
}
60 changes: 28 additions & 32 deletions src/permutations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,9 @@ enum PermutationState<Idx: ArrayOrVecHelper> {
/// No permutation generated yet.
Start { k: Idx::Length },
/// Values from the iterator are not fully loaded yet so `n` is still unknown.
Buffered { k: Idx::Length, min_n: usize },
Buffered { indices: Idx, min_n: usize },
/// All values from the iterator are known so `n` is known.
Loaded {
indices: Box<[usize]>,
cycles: Box<[usize]>, // TODO Should be Idx::Item<usize>
k: Idx::Length, // TODO Should be inferred from cycles
},
Loaded { indices: Box<[usize]>, cycles: Idx },
Copy link
Member

Choose a reason for hiding this comment

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

Is cycles really an Idx or should it actually be Idx::Item<usize>? I know that these are the same types, but don't the semantics differ? I.e. isn't Idx always meant to index into a slice, whereas Idx::Item<usize> is an array or a Vec containing usizes?

If we'd like to avoid answering this question, we could keep Loaded::k: Idx::Length.

Copy link
Contributor Author

@ronnodas ronnodas Jan 13, 2025

Choose a reason for hiding this comment

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

cycles is really neither an Idx nor an Idx::Item<usize>, since Idx::Item should be what is obtained by indexing something of type Idx into the buffer, but I don't feel strongly about this. Maybe it makes sense to keep cycles of type Box<[usize]> and switch back to the name PoolIndex so that that trait has clearer semantics.

/// No permutation left to generate.
End,
}
Expand Down Expand Up @@ -80,47 +76,43 @@ where
return None;
}
*state = PermutationState::Buffered {
k,
indices: Idx::start(k),
min_n: k.value(),
};
}
Some(Idx::item_from_fn(k, |i| vals[i].clone()))
}
PermutationState::Buffered { k, min_n } => {
PermutationState::Buffered { indices, min_n } => {
let k = indices.len();
if vals.get_next() {
// TODO This is ugly. Maybe working on indices is better?
let item = Idx::item_from_fn(*k, |i| {
vals[if i == k.value() - 1 { *min_n } else { i }].clone()
});
indices.borrow_mut()[k.value() - 1] += 1;
ronnodas marked this conversation as resolved.
Show resolved Hide resolved
*min_n += 1;
Some(item)
Some(indices.extract_item(vals))
} else {
let n = *min_n;
let prev_iteration_count = n - k.value() + 1;
let mut indices: Box<[_]> = (0..n).collect();
let mut cycles: Box<[_]> = (n - k.value()..n).rev().collect();
let mut cycles = Idx::from_fn(k, |i| n - 1 - i);
// Advance the state to the correct point.
for _ in 0..prev_iteration_count {
if advance(&mut indices, &mut cycles) {
if advance(&mut indices, cycles.borrow_mut()) {
*state = PermutationState::End;
return None;
}
}
let item = Idx::item_from_fn(*k, |i| vals[indices[i]].clone());
*state = PermutationState::Loaded {
indices,
cycles,
k: *k,
};
let item = Idx::item_from_fn(k, |i| vals[indices[i]].clone());
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this indices.extract_item(vals)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, for the same reason as in the PermutationState::Loaded case, indices is now of length n.

*state = PermutationState::Loaded { indices, cycles };
Some(item)
}
}
PermutationState::Loaded { indices, cycles, k } => {
if advance(indices, cycles) {
PermutationState::Loaded { indices, cycles } => {
if advance(indices, cycles.borrow_mut()) {
*state = PermutationState::End;
return None;
}
Some(Idx::item_from_fn(*k, |i| vals[indices[i]].clone()))
Some(Idx::item_from_fn(cycles.len(), |i| {
vals[indices[i]].clone()
}))
}
PermutationState::End => None,
}
Expand Down Expand Up @@ -173,22 +165,26 @@ impl<Idx: ArrayOrVecHelper> PermutationState<Idx> {
let total = (n - k.value() + 1..=n).try_fold(1usize, |acc, i| acc.checked_mul(i));
(total.unwrap_or(usize::MAX), total)
};
match *self {
match self {
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change from *self to self? Is this really realted to this commit?

We can take this, but I prefer not to mix it into these commits, if possible.

Copy link
Contributor Author

@ronnodas ronnodas Jan 13, 2025

Choose a reason for hiding this comment

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

It is related: Buffered.indices is no longer Copy, we could add a ref there instead.

Or alternatively also remove the refs in the Loaded case.

Self::Start { k } if n < k.value() => (0, Some(0)),
Self::Start { k } => at_start(n, k),
Self::Buffered { k, min_n } => {
Self::Start { k } => at_start(n, *k),
Self::Buffered { indices, min_n } => {
let k = indices.len();
// Same as `Start` minus the previously generated items.
size_hint::sub_scalar(at_start(n, k), min_n - k.value() + 1)
}
Self::Loaded {
ref indices,
ref cycles,
k: _,
} => {
let count = cycles.iter().enumerate().try_fold(0usize, |acc, (i, &c)| {
acc.checked_mul(indices.len() - i)
.and_then(|count| count.checked_add(c))
});
let count = cycles
.borrow()
.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)),
Expand Down