Implement CIEDE2000 color difference for Lab/Lch

[okaneco]

Add ColorDifference trait
Add csv data and test

Closes #143

[Ogeon]

Cool! It's going to take me a bit to review the math heavy part. But I appreciate the added tests a lot! That's always nice.

My understanding is that there's a conversion to Lch somewhere in there. I wonder if it would be possible to piggyback on that fact and implement it for both types at the same time. Maybe even simplify it a bit. Considering they are essentially the same color space, I would suggest implementing it for both in either case.

Other than that, you may want to run it through rustfmt (I'm always doing that on save, so it will happen at some point), if you haven't already. I saw you fiddled with the formatting, so I'm just assuming you may not have done it yet. 🙂

[okaneco]

The original Lab implementation took a few iterations and was harder than I expected. I started with trying to port the Wiki equations but consulted this paper and http://brucelindbloom.com/index.html?Eqn_DeltaE_CIE2000.html to clarify some details.

1. Formatting

• I didn't read documentation well enough to see that rustfmt can format one file. I use cargo fmt for personal projects and thought it was an alias for rustfmt. Since it did the whole project I only did it periodically when working with palette since it touches 29 files and would undo the modifications to files that weren't related to the commit. Do you have plans to do a formatting run on the whole project? I won't make this mistake again but this might encourage contributors to format with less resistance if they only know cargo fmt like I did. When I was looking through old issues I saw this comment and the following but some time has passed since then Make all colors generic over f32 and f64 #17 (comment).
• The other issue with formatting is I didn't want to obfuscate the math formulas too much so they could be reviewed. I tried to stay around 80 chars and planned on formatting before merge. However, formatting can mangle intentions when trying to balance the brackets and make verifying the equations more difficult while writing. An example of this is the formatting of t af27e55#diff-ede630151cf97a0b51375f57e5834cacR380

2. Lch
   Yes, it was interesting that there is a need for a*, b*, chroma, and h in the calculation. However, I'm not sure it simplifies much since the conversion occurs early but I'm open to any suggestions. I implemented it for lch.rs and calculated the a* and b* with the From for Lab implementation

[okaneco]

The build fails on the Linux CI machines but passes on Mac. On my current Windows desktop it passes cargo test --features strict for stable and nightly. The Lch result seems quite far off compared to the other platforms which passed but it's floating point and I know that can vary in implementation. This varied more than I expected.

failures:
---- convert::color_difference_ciede stdout ----
thread 'convert::color_difference_ciede' panicked at 'assert_relative_eq!(result_lch, expected.delta_e, epsilon = 0.001)
    left  = 4.7460666
    right = 4.8045
', palette/tests/convert/data_ciede_:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
failures:
    convert::color_difference_ciede
test result: FAILED. 11 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

---

Page 24 of the paper (4 of the pdf) has the test data (expecting 4.8045). It's either case 13 or 14. Not to say this is the direct cause, those cases are part of this testing category:

3. The CIELAB pairs labeled 7–16 test that the arctangent
computation for the determination of the hue in Eq. (7)
and the computation of mean hue in Eq. (14) are performed correctly.

[Ogeon]

I do appreciate all the hard work you put into this! I hope my previous comment didn't come out as dismissive. I was tired and not in shape for reading calculations.

Regarding the formatting, I see your point, and doing a formatting run may be a good idea. That should probably be done periodically, even, as not everyone uses rustfmt. And I did not mean to imply that you did anything wrong. I suspected it would have a large impact, so I wanted to bring it up now instead of accidentally reformatting it later...

There's an attribute you can add to the function to skip formatting there. That will preserve your custom formatting locally. It's much better than the alternative.

As for using Lch to make it simpler, that was just a guess. I wasn't sure how much of the conversion would be possible to reuse.

That error you found is suspiciously large, yes. I have seen differences between the Mac and Linux runners before, though. That's why approximate equality is often used. But one would think the error would be smaller... If it's the test you quoted, it sounds like Linux has a less accurate atan. I'm not sure what to do, really.

[okaneco]

No worries, I didn't read it that way and I learned something from it 👍

I added an attribute to skip formatting the get_color_difference() functions. I'll still use formatting but selectively revert changes that affect clarity. For the whole project, most of the formatting changes seemed to be adding whitespace to documentation and wrapping comment lines.

For the floating point, I can imagine there's a large amount of truncation or rounding due to the extra conversions involved. For the Lch test, I convert the original Lab color data to Lch. Then during get_color_difference() the a* and b* are calculated again. The frustrating part is that it works on other platforms, it seems strange there'd be the discrepancies or some possible optimization involved.

// CIEDE2000 test
for expected in data.iter() {
        let result_lab = expected.c1.get_color_difference(&expected.c2);
        assert_relative_eq!(result_lab, expected.delta_e, epsilon = 0.001);

        let lch1 = Lch::from(expected.c1);
        let lch2 = Lch::from(expected.c2);
        let result_lch = lch1.get_color_difference(&lch2);
        assert_relative_eq!(result_lch, expected.delta_e, epsilon = 0.001);
}

It's not ideal but could we have a larger epsilon in the Lch test for tests run on Linux? Beyond that, I'm not sure what else I can do to determine the root cause that would be worth the time.

[Ogeon]

Oh, so the error is only for Lch? Yes, that could be caused by too much conversion back and forth, I guess, combined with precision problems. Or if there's a mistake in the conversion itself, that only manifests itself on Linux because of some weird synergy. It's probably good to double check the conversion results with an external source, just to be safe. Maybe converting in f64 may help. Or maybe do the conversions on Windows or Mac, and add them to the test data, assuming they are more correct. Worst case, we will have to accept a worse result for Lch in the test, for the time being. I guess it's in the ballpark, at least.

Something else I was about to suggest, is to split the code into one part that gets the parameters from each format, and then sends them into a shared function. If we can isolate the "preparation" part from the parts they have in common, I think both maintenance and trouble shooting will be easier.

[okaneco]

f64 worked!

Separating the parameters from the calculation is a good suggestion to do anyway. I will try to move the main calculation into a shared function which accepts parameters from preparation functions.

[Ogeon]

Great! 🎉 It's probably good to add a small comment that explains why the test is performed like that, just for future adventurers. It's easy to forget things like this. Other than that, I think merging the implementations would be the last thing that's left to do.

[okaneco]

I separated the shared code to its own function but I wasn't sure of the best way for it to be shared by both Lab and Lch.

[Ogeon]

You can make a private color_difference module and put the function and trait there. Then you can just export the trait from the crate root. It would also be appropriate to name the function after the algorithm in that case. There may be more of them down the line.

[Ogeon]

Alright, nice! I think it's probably good to squash all of this now when there are a few edits. It's self contained enough for a single commit. Then I'm happy to merge when you are.

[okaneco]

Did you have more to review or I should squash it now?

[Ogeon]

I looked through it just now and I think it's good to go. I'm not going to go into details about naming and formatting here. Translating formulas to code is always weird an it's always possible to polish it later.

[okaneco]

I understand. Science and math programming tends to be ugly in my experience, especially with formulas as complicated as this. I'd follow any guidelines or preferences you have in the future but didn't see any immediate examples in the codebase. I leaned toward explicitness and trying to stay close to the variables as I transcribed them knowing they can be safely cleaned up later. I'm glad it works and thanks for the guidance.

[Ogeon]

I agree and it makes it easier to compare to the reference material. I prefer to start like this and improve incrementally. You are welcome to come back to this and iterate on it if you want to, but I think this is good for version 1.

Thanks for taking the time to implement it! It's very much appreciated!

bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push