Implement interleave() by laumann · Pull Request #405 · rayon-rs/rayon

laumann · 2017-09-03T13:06:11Z

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 · 2017-09-11T14:18:58Z

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 · 2017-09-11T16:55:01Z

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 · 2017-09-12T04:44:29Z

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

cuviper · 2017-09-17T21:21:32Z

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

laumann · 2017-09-18T07:25:36Z

Sure thing

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

Remove unused import, remove commented test code and add more test cases

laumann · 2017-09-18T07:44:30Z

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

This feature wasn't present in Rust 1.12.

laumann · 2017-09-18T08:58:11Z

@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 · 2017-09-18T16:27:49Z

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 · 2017-09-18T18:28:09Z

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 · 2017-09-18T18:40:53Z

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 · 2017-09-18T18:45:10Z

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 · 2017-09-18T19:05:01Z

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 · 2017-09-18T19:06:24Z

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

The flag indicating the next iterator to pull from was almost carried through all the way, except for InterleaveSeq's constructor, which always set it to false.

laumann · 2017-09-18T19:26:14Z

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

cuviper · 2017-09-19T18:49:44Z

src/iter/interleave.rs

+{
+    i: I,
+    j: J,
+    flag: bool,


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

I think you're right

cuviper · 2017-09-19T18:52:52Z

src/iter/interleave.rs

+    j: J,
+    i_len: usize,
+    j_len: usize,
+    flag: bool,


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... :)

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

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

toggle_flag

next_iter/next_producer

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)

I went with i_next.

cuviper · 2017-09-19T18:57:50Z

src/iter/interleave.rs

+        self.flag = !self.flag;
+        if self.flag {
+            match self.i.next() {
+                None => self.j.next(),


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.

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

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.

Ok, I added Fuse to both iterators in InterleaveSeq

cuviper · 2017-09-19T18:58:03Z

src/iter/interleave.rs

+pub struct InterleaveSeq<I, J> {
+    i: I,
+    j: J,
+    flag: bool


Again, better name and comments please.

cuviper · 2017-09-19T18:59:11Z

src/iter/interleave.rs

+
+    fn size_hint(&self) -> (usize, Option<usize>) {
+        let (ih, jh) = (self.i.size_hint(), self.j.size_hint());
+        let min = ih.0.checked_add(jh.0).unwrap_or(usize::MAX);


Perhaps saturating_add instead?

cuviper · 2017-09-19T19:01:30Z

src/iter/interleave.rs

+{
+    #[inline]
+    fn next_back(&mut self) -> Option<I::Item> {
+        if self.i.len() <= self.j.len() {


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

I agree, done.

cuviper · 2017-09-19T19:04:24Z

src/iter/mod.rs

        zip::new(self, zip_op.into_par_iter())
    }

+    /// Interleave elements of this iterator and the other given iterator.


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

Elaborated and added example.

cuviper · 2017-09-19T19:07:04Z

src/iter/test.rs

+        xs.par_iter().interleave(&ys).map(|&i| i).collect_into(&mut res);
+        assert_eq!(expected, res, "Case {} failed", i+1);
+    }
+}


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

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

Documentation in a few places, add tests with `rev()` to exercise `next_back()` and fix the `next_back()` implementation.

- Remove ExactSizeIterator from Iterator impl for InterleaveSeq - Use `saturating_add()` instead of `checked_add().unwrap_or(MAX)`

- Rename `flag` to `i_next` - Use `Fuse` for the iterators embedded in `InterleaveSeq`

cuviper · 2017-09-22T01:04:17Z

Let's merge -- thanks!

laumann · 2017-09-22T06:58:39Z

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

cuviper · 2017-09-22T17:11:54Z

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

laumann · 2017-09-22T19:58:05Z

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 · 2017-09-22T20:17:08Z

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 · 2017-09-22T20:30:13Z

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 · 2017-09-22T20:42:43Z

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)

Thomas Jespersen added 3 commits September 18, 2017 09:42

Implement interleave()

0694518

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

interleave: A little cleanup

35624e7

Remove unused import, remove commented test code and add more test cases

interleave: Remove more unused imports

bb6017b

Thomas Jespersen added 2 commits September 18, 2017 10:08

interleave: Don't use shorthand field initialization

1537988

This feature wasn't present in Rust 1.12.

interleave: Import std::cmp

4c2272d

interleave: Carry the flag through

924da1f

interleave: Carry self.flag through in InterleaveSeq

108d4a4

The flag indicating the next iterator to pull from was almost carried through all the way, except for InterleaveSeq's constructor, which always set it to false.

cuviper requested changes Sep 19, 2017

View reviewed changes

Thomas Jespersen added 3 commits September 20, 2017 22:31

interleave: Handle most review comments

7d7603b

Documentation in a few places, add tests with `rev()` to exercise `next_back()` and fix the `next_back()` implementation.

interleave: More review changes

a577184

- Remove ExactSizeIterator from Iterator impl for InterleaveSeq - Use `saturating_add()` instead of `checked_add().unwrap_or(MAX)`

interleave: Review changes

8fb112c

- Rename `flag` to `i_next` - Use `Fuse` for the iterators embedded in `InterleaveSeq`

cuviper approved these changes Sep 22, 2017

View reviewed changes

cuviper merged commit 2cf5ac2 into rayon-rs:master Sep 22, 2017

laumann deleted the add-interleave branch September 22, 2017 10:05

laumann mentioned this pull request Sep 22, 2017

implement interleave and interleave_shortest from itertools #384

Closed

Conversation

laumann commented Sep 3, 2017

Uh oh!

laumann commented Sep 11, 2017

Uh oh!

cuviper commented Sep 11, 2017

Uh oh!

laumann commented Sep 12, 2017

Uh oh!

cuviper commented Sep 17, 2017

Uh oh!

laumann commented Sep 18, 2017

Uh oh!

laumann commented Sep 18, 2017

Uh oh!

laumann commented Sep 18, 2017

Uh oh!

cuviper commented Sep 18, 2017

Uh oh!

laumann commented Sep 18, 2017

Uh oh!

cuviper commented Sep 18, 2017

Uh oh!

laumann commented Sep 18, 2017

Uh oh!

cuviper commented Sep 18, 2017

Uh oh!

laumann commented Sep 18, 2017

Uh oh!

laumann commented Sep 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cuviper commented Sep 22, 2017

Uh oh!

laumann commented Sep 22, 2017

Uh oh!

cuviper commented Sep 22, 2017

Uh oh!

laumann commented Sep 22, 2017

Uh oh!

cuviper commented Sep 22, 2017