Implement interleave()

[laumann]

For parity with itertools, interleave() should alternately produce
elements from two given iterators. Once one of the iterators run out,
elements should just be drawn from the other until both are exhausted.

This is from #384

[laumann]

I'm not sure why this fails, some of the errors are that the ? operator is not stable. Is there something I need to do?

[cuviper]

It's not you, but futures update -- see #412.

At a glance, the PR looks great. I'll try to give it a proper review soon!

[laumann]

Ok, thank you :-) Let me know if there's anything I can do...

[cuviper]

Can you now please rebase, to see how CI fares?

[laumann]

Sure thing

[laumann]

Sorry about the noise, I rebased on a different machine, and forgot to set core.name and core.email - should be fixed now.

[laumann]

@cuviper It should build now, I installed a rust 1.12.0 locally. The commitment to backwards compatibility is pretty cool, I think. 🤞 for tests passing

[cuviper]

Looks like the order is getting mixed up:

---- iter::test::check_interleave_eq stdout ----
	thread 'iter::test::check_interleave_eq' panicked at 'assertion failed: `(left == right)`
  left: `[0, 10, 1, 2, 11, 12, 3, 13, 4, 14, 5, 15, 6, 7, 16, 17, 8, 18, 9, 19]`,
 right: `[0, 10, 1, 11, 2, 12, 3, 13, 4, 14, 5, 15, 6, 16, 7, 17, 8, 18, 9, 19]`', src/iter/test.rs:1735:4

[laumann]

Yeah, I'm not sure why though - I can't get it to fail locally, and it seems to be only on nightly that it fails :-/

[cuviper]

it seems to be only on nightly that it fails

nightly is the only one that runs tests, if only because the dev-dependency compiletest_rs requires it (and dev-deps can't be conditional).

[laumann]

Hmm, that makes sense - am I doing something wrong then? I run cargo +nightly test, and I tried setting RUSTFLAGS='--cfg=rayon_unstable' as well, but so far no dice.

[cuviper]

I think Travis runs with 2-cpus, so setting RAYON_NUM_THREADS=2 locally may get you closer. I suggest adding a temporary debug-print with all your index, i_idx, j_idx, i_split, j_split, flag, even etc. so you have a trace of how things go awry.

[laumann]

Oh, that did it, thanks! I'll do some debugging now :-)

[laumann]

Thanks for the help @cuviper! I had forgotten to carry the flag through to InterleaveSeq's constructor.

[cuviper]

Do we really need the flag here? I think it's not used until we get to the Producer.

[laumann]

I think you're right

[cuviper]

I think we need a more informative name for this flag, and a doc-comment explaining its purpose as well. Think of the poor developer who may have to revisit this code many months later... :)

[laumann]

I'll try to think of a better name... names are hard :-)

[laumann]

I'm open for suggestions on the name :-) Any of these you'd prefer?

• toggle_flag
• next_iter/next_producer

[cuviper]

It's indicating which iterator will produce the next item, so maybe i_next or j_next? (depending on what you want true to mean)

[laumann]

I went with i_next.

[cuviper]

This sort of assumes that i will be fused (continue returning None), as the flag flips back and forth trying this iterator again. We may want a done: bool to remember and stop flipping the flag, or I guess len() can tell us this too.

[laumann]

Yes, the itertools version is fused. I'm not really sure of the best way to handle it, I would just add a done bool I think

[cuviper]

Ah, it's literally fused! You could just use that too. Fuse has a done bool itself, but it also specializes on the unstable FusedIterator trait to skip that check for naturally-fused iterators.

[laumann]

Ok, I added Fuse to both iterators in InterleaveSeq

[cuviper]

Again, better name and comments please.

[cuviper]

Perhaps saturating_add instead?

[laumann]

Done.

[cuviper]

I think if they are equal length, then it needs to take the flag into account.

[laumann]

I agree, done.

[cuviper]

Your PR is more descriptive than these docs -- please elaborate! An example would be nice too.

[laumann]

Elaborated and added example.

[cuviper]

How about a test under rev(), for the next_back behavior?

[laumann]

I added another assert_eq that tests each case when using rev()

[cuviper]

Let's merge -- thanks!

[laumann]

Thanks! It should be relatively easy to implement interleave_shortest, I think, so I could give that a shot if you're interested.

[cuviper]

Yeah, I think interleave_shortest will be a simple wrapper, much like zip_eq and zip. Go for it!

[laumann]

Hmm, it may not be as trivial as I thought - the zip_eq is simple, because it is just zip with an added assertion in front. interleave_shortest should terminate once one of the iterators runs out (interleave exhausts them both).

But by their example, it is not simply take()ing the first n elements where n = cmp::min(i.len(), j.len()):

let it = (1..7).interleave_shortest(vec![-1, -2]);
itertools::assert_equal(it, vec![1, -1, 2, -2, 3]);

(source)

Observe that the last element is drawn from the first iterator, ie three elements are drawn from that one while the second only provides two elements.

I can see how I'd change with_producer for Interleave to select the right lengths for creating the producers, but I'd like to avoid copying too much code if possible. Maybe I'll try to pull that code out in interleave.rs and implement InterleaveShortest in interleave.rs? But I'm for better suggestions on how to implement this :-)

[cuviper]

Something like this?

if i.len() <= j.len() {
    // take equal lengths from both iterators
    let n = i.len();
    i.take(n).interleave(j.take(n))
} else {
    // take one extra item from the first iterator
    let n = j.len();
    i.take(n + 1).interleave(j.take(n))
}

(using take even when it's redundant, to get consistent types)

[laumann]

Thanks for the suggestion! I was trying something similar, but run into:

error[E0308]: mismatched types
  --> src/iter/interleave.rs:49:9
   |
42 | pub fn new_shortest<I, J>(i: I, j: J) -> Interleave<I, J>
   |                                          ---------------- expected `iter::interleave::Interleave<I, J>` because of return type
...
49 |         i.take(n).interleave(j.take(n))
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected type parameter, found struct `iter::take::Take`
   |
   = note: expected type `iter::interleave::Interleave<I, J>`
              found type `iter::interleave::Interleave<iter::take::Take<I>, iter::take::Take<J>>`

error[E0308]: mismatched types
  --> src/iter/interleave.rs:53:9
   |
42 | pub fn new_shortest<I, J>(i: I, j: J) -> Interleave<I, J>
   |                                          ---------------- expected `iter::interleave::Interleave<I, J>` because of return type
...
53 |         i.take(n + 1).interleave(j.take(n))
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected type parameter, found struct `iter::take::Take`
   |
   = note: expected type `iter::interleave::Interleave<I, J>`
              found type `iter::interleave::Interleave<iter::take::Take<I>, iter::take::Take<J>>`

[cuviper]

pub fn new_shortest<I, J>(i: I, j: J) -> Interleave<I, J>

I think we'll want this to return a distinct InterleaveShortest<I, J> type, so we're not bound to any particular implementation details. This custom type can contain an Interleave<Take<I>, Take<J>>, and forward all methods to that. (as ZipEq does)