Itertools::multi_cartesian_product

[tobz1000]

Hello!

I have created a new Itertools adaptor: MultiProduct. It is to be used on a meta-iterator (iterator which yield iterators) to produce the cartesian product of the iterators it yields. It is created by calling the .multi_cartesian_product() method.

This can be used, instead of the iproduct macro, when the number of iterators is not known at compile time.

It currently uses a recursive function, which makes it susceptible to stack overflows for products with a huge number of elements. I could probably rework this to use a heap stack if desired, although I haven't thought about it too much so far.

It adds 7 new top-level items to adaptors/mod.rs, so maybe it'd be worth splitting into its own file?

Any comments/questions, please let me know.

[bluss]

Yes, please use its own file. Will review later, not sure when I have time.

[tobz1000]

The 1.12 build is failing because of (at least) a syntax feature added in 1.17. Is there a reason that 1.12 is targeted?

[bluss]

It's the current minimum version for itertools. Being a widely used library, we don't want to change that if we can avoid it.

[bluss]

Thanks for working on this! This is a worthwhile feature that fits well inside the scope of itertools.

There are several items that I'd like to be addressed with this code before it can be merged. Perhaps we can find a person from the community that can review it closer?

Specifically look at

• Avoid .collect() and making new vectors unless absolutely necessary, for example in .last().
• Avoid .unwrap() unless absolutely necessary. Prefer pattern matching and using the Some/None-ness for control flow
• We need to have an eye towards fuse-correctness
• Please use quickcheck tests; we cover many more cases that way, without having to write a lot of cases by hand

[tobz1000]

Thanks for taking the time to look at this.

• Avoid .collect() and making new vectors unless absolutely necessary, for example in .last().

I've reworked .last to only collect once. I think, even in the case of returning None, this vec is unavoidable since it is unknown that we will need to throw away the result beforehand.

• Avoid .unwrap() unless absolutely necessary. Prefer pattern matching and using the Some/None-ness for control flow

After modifying .last, the only remaining unwrap is in .curr_iterator(). Unfortunately, this method does rely on all MultiProductIter.curs being left in the Some state after recursively calling MultiProduct::iterate_last. I haven't been able to find an elegant way to get rid of this potential invalid state.

• We need to have an eye towards fuse-correctness

Could you elaborate on how it should behave please? Currently, at the end of iteration, a MultiProduct is back in its starting state. It doesn't rely on the (un-)fusedness of its child iterators, since it uses a fresh clone of each (similarly to Product). Would you like it to become automatically fused if the child iterators are fused?

• Please use quickcheck tests; we cover many more cases that way, without having to write a lot of cases by hand

I will look into this - I'm not quite sure what quickcheck achieves yet, but I'll do some research :)

I've done some thinking about this adaptor API-wise, and could use your input: would we perhaps want to yield a wrapper struct which implements Iterator, rather than a naked vec? The implementation would still use vecs underneath, but this could potentially save a breaking change in future. To complement this, a custom FromIterator for vec would save the user having to copy the vec if that's what they really want.

[bluss]

Great to see the progress

Would you like it to become automatically fused if the child iterators are fused?

This is not even necessary. Fuse-preserving is good but we can't always have it. The most important part is to not call .next() on any iterator we have already gotten a None from. For .fuse()d iterators and known-good ones we don't need this rule.

See qc tests in tests/quick.rs

[tobz1000]

I've added some quickcheck tests equivalent to those already present for cartesian_product/iproduct, plus a test for .last(). They take a new Iterator, ShiftRange, which yields a number of Iters, each of which has different start/end values.

These seem to cover everything I was previously testing for in test_std.rs, so those tests have been removed.

Additionally, I've added some benchmarks to compare to iproduct. The results are roughly a 30x slowdown. :)

(Re: my suggestion to yield an iterable rather than a naked vec: I've realised that there is no type parameter to specify the type of Iterator you're collecting from :) so for the purpose of saving unnecessary memory copies, yielding vecs is probably best.)

[bluss]

It is not allowed to implement ESI since part of its contract is that the size of the iterator fits inside usize::max_value() (and we can't ensure that). That kind of small change can be adjusted after merge.

[bluss]

Thanks for your solid work on this!

[bluss]

changes unrelated to the PR in this file unfortunately (such should never be in the PR)

[bluss]

Could you do me a favour and clean up the commit history in the PR? how many commits it ends up with is not so imporant. If you don't do that, I can squash merge it in the end instead.

[tobz1000]

Sure, I'll try and address your issues in the next day or so.

[tobz1000]

Addressed your comments, removed some unnecessary casts, and squashed and rebased. You can see the latest actual changes here.

[bluss]

Thank you!