Make Take iterator for gradient inclusive of both end colors, add tests

[okaneco]

As mentioned in the third point of #156, this PR includes the bounds of the gradient along with more equal distribution of colors returned from the iterator.

In the current version of the iterator, (self.from_head) / (self.len) will approach 1 but never equal 1 which results in the upper gradient bound being excluded. The current behavior of take() is surprising if one expects the last color step to be the max color of the gradient. This PR increases step distance so the user receives an iterator of colors that evenly traverses from the minimum bound of the gradient to the maximum. The step is increased by subtracting 1 from self.len in the calculation of i. The calculation for i will then include

• 0 when self.from_head is 0
• 1 when self.from_head == self.len - 1.

The assignment of i has a conditional for when self.len is 1. This avoids division by 0 which results in NaN parameters for the iterator color returned in the case of take(1). The check needed to be added to the DoubleEndedIterator, otherwise it would NaN on 1 as well. The behavior of take(1) before this PR is to supply the minimum color in the gradient.

Pictured below is an example of the difference between current behavior and this PR in 3 sets of Lch gradients. The 1st, 3rd, and 5th rows are the current implementation of the iterator and the others are from the PR with the final step being inclusive of the maximum gradient color specified. The gradients are no longer skewed towards the beginning. The penultimate steps are very close to the current behavior's final step.

---

gradient::test::simple_slice fails as a result of this PR.

---- gradient::test::simple_slice stdout ----
thread 'gradient::test::simple_slice' panicked at 'assert_relative_eq!(t1, t2)

    left  = Rgb { red: 0.8888888888888888, green: 0.0, blue: 0.1111111111111111, standard: PhantomData }
    right = Rgb { red: 0.875, green: 0.0, blue: 0.125, standard: PhantomData }

', palette/src/gradient.rs:453:13

#[test]
    fn simple_slice() {
        let g1 = Gradient::new(vec![
            LinSrgb::new(1.0, 0.0, 0.0),
            LinSrgb::new(0.0, 0.0, 1.0),
        ]);
        let g2 = g1.slice(..0.5);

        let v1: Vec<_> = g1.take(10).take(5).collect();
        let v2: Vec<_> = g2.take(5).collect();
        for (t1, t2) in v1.iter().zip(v2.iter()) {
            assert_relative_eq!(t1, t2);
        }
    }

[okaneco]

The simple slice test passes when

let v1: Vec<_> = g1.take(10).take(5).collect();

is changed to

let v1: Vec<_> = g1.take(9).take(5).collect();

The size of the step is different so the test failed. Here are g1.take(10).take(5), g2.take(5).

g1.take(10).take(5)
Rgb { red: 1.0, green: 0.0, blue: 0.0, standard: PhantomData }
Rgb { red: 0.8888888888888888, green: 0.0, blue: 0.1111111111111111, standard: PhantomData }
Rgb { red: 0.7777777777777778, green: 0.0, blue: 0.2222222222222222, standard: PhantomData }
Rgb { red: 0.6666666666666667, green: 0.0, blue: 0.3333333333333333, standard: PhantomData }
Rgb { red: 0.5555555555555556, green: 0.0, blue: 0.4444444444444444, standard: PhantomData }
g2.take(5)
Rgb { red: 1.0, green: 0.0, blue: 0.0, standard: PhantomData }
Rgb { red: 0.875, green: 0.0, blue: 0.125, standard: PhantomData }
Rgb { red: 0.75, green: 0.0, blue: 0.25, standard: PhantomData }
Rgb { red: 0.625, green: 0.0, blue: 0.375, standard: PhantomData }
Rgb { red: 0.5, green: 0.0, blue: 0.5, standard: PhantomData }
simple slice test

g1.take(9).take(5) produces the same results as g2.take(5) and passes the test.

g1.take(9)
Rgb { red: 1.0, green: 0.0, blue: 0.0, standard: PhantomData }
Rgb { red: 0.875, green: 0.0, blue: 0.125, standard: PhantomData }
Rgb { red: 0.75, green: 0.0, blue: 0.25, standard: PhantomData }
Rgb { red: 0.625, green: 0.0, blue: 0.375, standard: PhantomData }
Rgb { red: 0.5, green: 0.0, blue: 0.5, standard: PhantomData }
Rgb { red: 0.375, green: 0.0, blue: 0.625, standard: PhantomData }
Rgb { red: 0.25, green: 0.0, blue: 0.75, standard: PhantomData }
Rgb { red: 0.125, green: 0.0, blue: 0.875, standard: PhantomData }
Rgb { red: 0.0, green: 0.0, blue: 1.0, standard: PhantomData }
g1.take(9).take(5)
Rgb { red: 1.0, green: 0.0, blue: 0.0, standard: PhantomData }
Rgb { red: 0.875, green: 0.0, blue: 0.125, standard: PhantomData }
Rgb { red: 0.75, green: 0.0, blue: 0.25, standard: PhantomData }
Rgb { red: 0.625, green: 0.0, blue: 0.375, standard: PhantomData }
Rgb { red: 0.5, green: 0.0, blue: 0.5, standard: PhantomData }

I've amended the simple_slice_test. The assert_relative_eq!() can be made into assert_eq!() and still pass the test. Not sure if this is worth changing.

[Ogeon]

Awesome, thanks! And nice idea with the comparison image! 👍 I would also appreciate it a lot if you can update the documentation to clarify that take is inclusive now.

The assert_relative_eq!() can be made into assert_eq!() and still pass the test. Not sure if this is worth changing.

I would rather do the opposite. Float equality is not reliable and it may fail on some systems but not on other, due to different rounding and such.

[Ogeon]

9 items is 8 "jumps" and 5 items is 4 "jumps". 5 ends at the middle of 9. It's slightly mind bending (for my post work Friday brain), but it checks out.

[okaneco]

Yes, I had to use pen and paper to reason about it. My thinking was the interval between each color step is 1/(n-1) for n > 1. So 9 is 1/8, 10 is 1/9, etc.

When you take 9, you're getting 7 intermediate colors that are 8 "jumps" apart. At first, this can be confusing when reasoning about what to take if you want a specific interval. I wanted to add a short but hopefully clear explanation of this in the docs so future people can avoid bending minds.

[okaneco]

Thanks for the feedback. I've updated the documentation in gradient.rs, let me know if there are any other places to mention or corrections to be made. Terminology around explaining the change feels very clunky but did my best to streamline it.

I hope I understood your comment. Now, the logic is more explicit about the special cases and the interpolation is isolated. The "let if" was opaque and not ideal.

[okaneco]

I updated the readme to mention an example in an updated /gfx/readme_gradients.png. However I wasn't sure if the readme_examples were automatically built at some point. I copied the relevant gradient portion into a local test file to work out, then I reinserted the gradient sub_image portion and ran the --features strict test to make sure it built.

[Ogeon]

I think the new special casing is much easier to follow. 👍 I wrote some suggestions for the documentation, but otherwise it's just that cast(0) that looks suspicious.

I updated the readme to mention an example in an updated /gfx/readme_gradients.png. However I wasn't sure if the readme_examples were automatically built at some point.

You will have to update the example images to the gfx folder. It's not automatic. I have been considering the. I haven't gotten around to find a good system for validating the readme examples, but the output from the example program should be the same as the images in gfx, and the code should be the same as in the readme. It would have been much easier if it didn't involve any graphics...

[okaneco]

I had some trouble getting the macro for assert_relative_eq to work in the doc test example. It ended up looking more like the unit tests.

[Ogeon]

That's fine. I just didn't remember if the required trait was implemented for Vec, so I took a chance.

I think this look good to go now! Do you want to squash any of the fixups before it's merged? Otherwise I'll order it to be merged as soon as I get to it tomorrow. 🙂

[Ogeon]

(sorry, clicked the wrong button there)

[okaneco]

Great 👍

Do you want to edit the PR title or original comment to mention anything?

[Ogeon]

No, I think it's fine the way it is.

bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push