Fixing Issue 152

[ChristopherDavenport]

Working as group which is why the pull request is being initiated early so that we can each merge into this common branch.

First Commit Implements cmp
Uses the existing zip, map, and reduce combinators to implement cmp.

Closes #152

[nikomatsakis]

Looks good so far: one thing that's missing is tests =)

[ChristopherDavenport]

This is a basic implementation where performance can be improved through early returns, however this should effectively satisfy the contracts.

[nikomatsakis]

Thanks for the PR! It's close but I think there are a few things to fix; I left some comments.

[nikomatsakis]

I think this is backwards. You want if cmp_a == Ordering::Equal { cmp_ b } else { cmp_a }.

[nikomatsakis]

This is also not quite right. I think you want something like this:

match cmp_a {
    Some(Ordering::Equal) => cmp_b,
    Some(_) => cmp_a,
    None => None,
}

The idea is that once we find something which is ordered (and not equal), it doesn't matter what comes after. But if we find something unordered, same is true.

Here is an example demonstrating this: http://play.integer32.com/?gist=87daf4489fb2ca744e1ea99abac0f443&version=stable

[nikomatsakis]

These tests are actually not testing your code. You are building vectors and then comparing them sequentially. You need something like:

let a = (0..1024).into_par_iter(); // NB: not collect() :)
let b = (0..1024).into_par_iter();
let result = a.eq(b);
assert!(result);

However, my preference is to write the tests so that they compare par against seq iterators, rather than directly encoding the expected result:

assert_eq!((0..1024).into_par_iter().eq(0..1024), (0..1024).eq(0..1024));

[ChristopherDavenport]

I'll add tests both between pars and between par and seq iterators. That way you'll have tests for expected behavior internally as well as expected behavior as compared to the external standard of seq iterators.

[nikomatsakis]

can you also add some tests involving floats (which are not fully ordered)?

http://play.integer32.com/?gist=87daf4489fb2ca744e1ea99abac0f443&version=stable would be a good one, if we added the parallel version and compared them against one another.

It'd also be good to add a test with some randomly generated data, using a fixed seed.

Something like this:

#[test]
pub fn cmp_and_partial_cmp() {
    use rand::{Rng, SeedableRng, XorShiftRng};

    let mut rng = XorShiftRng::from_seed([14159, 26535, 89793, 23846]);
    let a: Vec<i32> = rng.gen_iter().take(1024).collect();
    let b: Vec<i32> = rng.gen_iter().take(1024).collect();
    for i in 0..a.len() {
        assert_eq!(a[i..].par_iter().partial_cmp(b[i..]));
        assert_eq!(a[i..].iter().cmp(b[i..]);
    }
}

(a twist on @utkarshkukreti's test)

[ChristopherDavenport]

I'll get on it right away. I know the rest of the crew that was working on this with me went onto vacation, so I'll see what I can get under control.

As an aside, thank you for the really useful feedback. I'm fairly new to rust and such constructive criticism is very rare and very appreciated as I learned a lot from your change suggestions.

[ChristopherDavenport]

1. Fixed cmp, it was indeed backwards.
2. For partial_cmp the much simpler match semantics play out much easier and return correctly if an earlier value is greater.
3. All tests have been switched to into_par_iter or par_iter where appropriate. Tests check for appropriate evaluation as set by this library, and with comparison to seq iterators, therefore if seq iterator implementation in some way changes this will be apparent, as local tests will pass but comparison tests will fail.
4. Float vectors were analyzed like in the simple example suggested pulling both with an early NaN returning None, and after a difference not making a difference to the return. Randomly generated data using a fixed seed was implemented for both cmp and partial_cmp with a comparison to the sequential version showing that the comparisons are indeed equal.

If there is anything more I need to do just let me know.

[nikomatsakis]

Looks good.

[nikomatsakis]

Just FYI, the second call to par_iter (and iter) is not necessary: a[i..].par_iter().partial_cmp(&b[i..]) is equivalent. But this is also fine, no need to change it -- more explicit this way.

[nikomatsakis]

I see you have some merge conflicts -- do you think you can rebase? I suspect part of the problem is that you seem to have run rustfmt, which caused a lot of miscellaneous unrelated diffs in the code. (Though I don't mind, since I've been wanting to run rustfmt regularly).

One thing you might try if you are going to rebase is to introduce a first commit where you run rustfmt -- before you make any of our changes. That will isolate the rustfmt diffs into one commit and also make merging/rebasing easier.

Let me know though: if you are not that familiar with git, or don't have time, I can also help out and merge the changes.

[ChristopherDavenport]

I will run rustfmt on these files which has no issue and then issue a rebase of an intermediate commit.

I can do one across the whole project but it seems to create an oddity in the implementation of Tree in scope/test.rs. So rather than include it as the baseline after I finish the changes I can issue a pr with the whole project run through the rustfmt blender, that way the commits have good separation of concerns.

[nikomatsakis]

Thanks!