Array combinations #546
Array combinations #546
Conversation
There was a problem hiding this comment.
Tremendous thanks for submitting this PR. I apologize in advance for how long it's going to take me to review this; I'm in the home stretch of my dissertation and that's eating up my time.
I think I'm amenable to increasing the MSRV for const-generics in a major version bump, but it'd be great if we could identify any other parts of the library that'd also benefit from const generics first.
Certainly. Perhaps it's worth opening a tracking issue and a feature branch for 0.11? |
|
For now, I'm not in a rush to get this merged in. For my own use cases, noting the performance details above, I ended up making a macro to implement the for loop strategy simply for_combinations!([a, b, c] in slice {
println!("{:?} {:?} {:?}", a, b, c);
}):But I'm happy to look into the const generics major bump |
|
I've updated the implementation quite a bit. I've used Updated benchmark times: |
| #[derive(Debug, Clone)] | ||
| pub struct ArrayCombinations<I: Iterator, const R: usize> { | ||
| iter: I, | ||
| buf: Vec<I::Item>, |
There was a problem hiding this comment.
Wouldn't it be better to use an ArrayVec here, and so avoid heap allocations entirely?
There was a problem hiding this comment.
IIRC, buf caches the elements from iter, so I think it is impossible to impose an upper bound on the length here.
However, and if my suspicion is correct, I am thinking if we should actually use LazyBuffer here - unsure about that.
There was a problem hiding this comment.
Ah yes, quite right.
Actually, my current use-case is to generate pairs of items from an iterator over a large array; so buffering all items is quite costly... in this case, my iterator implements Clone so I can instead hold R clones of the iterator and entirely avoiding dynamic/heap allocations:
struct ArrayCombinations<I: Iterator, const R: usize> {
/// Each element of this array is an iterator that can be inspected for
/// the corresponding element of the resulting combo.
///
/// Peeking each iterator yields the last emitted value, such that
/// invoking `next()` first advances the state before emitting the
/// result. For example, given `[1,2,3,4,5].array_combinations::<3>()`,
/// the initial state of `iters` is such that peeking each iterator element
/// will yield `[1, 2, 2]` (becoming `[1, 2, 3]` after `next()` is called).
iters: [Peekable<I>; R],
}
impl<I, const R: usize> ArrayCombinations<I, R>
where
I: Iterator + Clone,
I::Item: Clone,
{
pub fn new(iter: I) -> Self {
let mut iters = ArrayVec::new();
iters.push(iter.peekable());
for idx in 1..R {
let last = iters.last_mut().unwrap();
if idx > 1 {
last.next();
}
let last = last.clone();
iters.push(last);
}
Self { iters: iters.into_inner().ok().unwrap() }
}
}
impl<I, const R: usize> Iterator for ArrayCombinations<I, R>
where
I: Iterator + Clone,
I::Item: Clone,
{
type Item = [I::Item; R];
fn next(&mut self) -> Option<Self::Item> {
'search: for idx in (0..R).rev() {
self.iters[idx].next();
if self.iters[idx].peek().is_some() {
let mut clone = self.iters[idx].clone();
for reset in self.iters[idx+1..].iter_mut() {
clone.next();
match clone.peek() {
Some(_) => *reset = clone.clone(),
None => continue 'search,
}
}
break;
}
}
self.iters
.iter_mut()
.map(Peekable::peek)
.map(Option::<&_>::cloned)
.collect::<Option<ArrayVec<_, R>>>()
.map(ArrayVec::into_inner)
.map(Result::ok)
.flatten()
}
}
An alternative to
TupleCombinationsthat works in a similar fashion to the standardCombinationsiterator. It's implemented using const generics and manages to work for >12 elements too. Another benefit is that it doesn't require the iterator to implementClone, onlyI::ItemI doubt I'll be able to get merged as is. Since this uses const generics, it would need to either be feature gated or we raise the MSRV from 1.32 all the way to 1.51
Benchmark results:
Interesting notes: c1 and c2 for this version are much higher than I would have expected but c3 and c4 are much lower than the tuple alternatives