Implement approx eq trait

[sidred]

• Implements approx eq trait for all the colors from the approx crate.
• Removed the assert_approx_equal macro and replaced all comparisons with the assert_relative_eq macro from the approx crate
• The equality macros in the integration tests were also replaced with the assert_relative_eq macro

For hue comparison, the recommendation is use 180 * epsilon as the epsilon and do not compare by ulps. This will cause a loss of precision for numbers close to zero, but for angle differences it is still reasonably small.

Check https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/ for more information.

[Ogeon]

Nice! Now, I haven't studied that trait yet, so I don't know all the details about it, but shouldn't those constants really come from the float type? I mean, different types have different precision, and so on.

[sidred]

To-Do

• Delete assert_approx_eq macro
• Convert all tests to use assert_rel_eq directly

Design

Implemented the approx trait separately for the hue types using angle difference and separate default epsilon and ulps for the angles. The hue based colors will use the angle defaults for angles and regular defaults for floats. One possible issue with this is that manually providing epsilon will not work as expected

let a = Lch::new(...);
let b = Lch::new(...);

// Will use separate precision for l and chroma vs hue based on the defaults
approx_rel_eq(a,b)

// Will use 0.0001 for even the hues
approx_rel_eq(a,b, epsilon = 0.0001)

//To manually set separate defautls use
approx_rel_eq(a.l,b.l, epsilon = 0.0001)
approx_rel_eq(a.chroma,b.chroma, epsilon = 0.0001)
approx_rel_eq(a.hue,b.hue, epsilon = 0.1)

Another alternative is to only check non hues as default as hues must be manually checked

let a = Lch::new(...);
let b = Lch::new(...);

// Will check only l and chroma. hue not checked
approx_rel_eq(a,b)
// To check hue
approx_rel_eq(a.hue,b.hue)

// Will use 0.0001 for l and chroma
approx_rel_eq(a,b, epsilon = 0.0001)

//To check separately need to use
approx_rel_eq(a,b, epsilon = 0.0001)
approx_rel_eq(a.hue,b.hue, epsilon = 0.1)

Option one seems simpler and that is currently implemented. Not sure if this needs to be documented somewhere.

[sidred]

Nice! Now, I haven't studied that trait yet, so I don't know all the details about it, but shouldn't those constants really come from the float type? I mean, different types have different precision, and so on

This does not work:

        impl<T:Float> ApproxEq for Xyz<T> {
            type Epsilon = T;

            fn default_epsilon() -> Self::Epsilon {
                flt(EPSILON) 
            }
            ....
            fn relative_eq(&self, other: &Self, epsilon: Self::Epsilon, max_relative: Self::Epsilon) -> bool {
                T::relative_eq(&self.x, &other.x, epsilon, max_relative)
            }
            ...
        }

errors with approxeq not implemented for T

[Ogeon]

I must say that alternative 2 seems very fishy, and may become a really bad surprise. Here I come with my questions again, but are the differences really necessary? Does the different scales really matter that much?

errors with approxeq not implemented for T

Because you need to write impl<T:Float + ApproxEq> ApproxEq for Xyz<T> {.

[sidred]

Because you need to write impl<T:Float + ApproxEq> ApproxEq for Xyz {.

Actually thats what I had used, but it errors with

src/xyz.rs:283:43: 283:50 error: mismatched types:
 expected `<T as approx::ApproxEq>::Epsilon`,
    found `T`
(expected associated type,
    found type parameter) [E0308]

src/xyz.rs:283         T::relative_eq(&self.y, &other.z, epsilon, max_relative)
                                                           ^~~~~~~

[Ogeon]

Did you write type Epsilon = T::Epsilon; or type Epsilon = <T as ApproxEq>::Epsilon;, as well? That is necessary to make sure Epsilon is the same type as for T.

[sidred]

I must say that alternative 2 seems very fishy, and may become a really bad surprise. Here I come with my questions again, but are the differences really necessary? Does the different scales really matter that much?

I am not sure. I am ok with having a single precision. I was looking at it from the tests point of view, where the data was not really precise, but that should not drive the design of the library.

We need to figure out the right defaults too. Currently set to

const EPSILON: f64 = 0.0001_f64;
const MAX_RELATIVE: f64 = 0.0001_f64;
const ULPS: u32 = 10;
const ANGLE_EPSILON: f64 = 0.1_f64;
const ANGLE_MAX_RELATIVE: f64 = 0.1_f64;
const ANGLE_ULPS: u32 = 10;

[Ogeon]

I was looking at it from the tests point of view, where the data was not really precise, but that should not drive the design of the library.

No, it's better to fix the tests or test with a lower precision.

We need to figure out the right defaults too.

Those will depend on the precision if the float type, so I would suggest that we try to reuse them. We can just delegate all the relevant methods to T, once all the types checks out, and see how it goes (I think it will be all fine). It's luckily very customizable.

[sidred]

Did you write type Epsilon = T::Epsilon; or type Epsilon = ::Epsilon;, as well? That is necessary to make sure Epsilon is the same type as for T.

Any ideas on how to cast epsilon as T

    fn default_epsilon() -> Self::Epsilon {
        flt(EPSILON)
    }

error: the trait `num::traits::Float` is not implemented for the type `<T as approx::ApproxEq>::Epsilon` [E0277]

[Ogeon]

You could require T::Epsilon to also be Float, like this: where <T as ApproxEq>::Epsilon: Float, but I'm sill voting for not having our own epsilons.

[sidred]

This works

    fn default_epsilon() -> Self::Epsilon {
        T::default_epsilon()
    }
    fn default_max_relative() -> Self::Epsilon {
        T::default_max_relative()
    }
    fn default_max_ulps() -> u32 {
        T::default_max_ulps()
    }

So we don't manually set the precision then, only override it when necessary.

[Ogeon]

Yes, I think that's more robust, since it will be adapted to the float type. It's also not the end of the world if it would turn out to be a problem.

[sidred]

We have an issue with the angles comparison. We are comparing to zero. https://randomascii.wordpress.com/2012/02/25/comparing-floating-point-numbers-2012-edition/. Look at the infernal zero section.

[sidred]

Based on the link we can multiply the epsilon by 180. For ulps not clear if multiplying by 180 is right.

    fn default_epsilon() -> Self::Epsilon {
        T::default_epsilon() * 180
    }
    fn default_max_relative() -> Self::Epsilon {
        T::default_max_relative() * 180
    }
    fn default_max_ulps() -> u32 {
        T::default_max_ulps() *180
    }

[Ogeon]

We have an issue with the angles comparison. We are comparing to zero.

Floats are clearly a piece of junk, sometimes. I'm not sure what we can do to make it better, since we have to touch the values in some way. Maybe check a.norm() == b.norm() || a.positive_norm() == b.positive_norm(). They will both give false negatives for different values (one at +-180 and the other at 0 and 360), but shouldn't give false positives. Using || should cancel out the false negatives.

Based on the link we can multiply the epsilon by 180. For ulps not clear if multiplying by 180 is right.

I guess that's alright, but I don't know about ULPs, either. They are a bit different...

[Ogeon]

If you are comparing two arbitrary numbers that could be zero or non-zero then you need the kitchen sink. Good luck and God speed.

😲

[Ogeon]

Sorry, wrong button... 😨

[sidred]

Basicaly what the link is saying is don't use ulps_eq for angles. only use rel_eq.

Some numbers in the link.
http://is.gd/G2ZJNr

180 * float is a good default. It is slightly larger than 1 ulps at 180.0

180 ulps at 0.0 is still very small, should we just multiply ulps also by 180 to be safe?

[Ogeon]

Might as well do that, as long as it doesn't end up being too big.

[sidred]

I've made a small change

This is an error

            fn default_max_epsilon() -> u32 {
                T::default_max_epsilon() * 180.0
            }
// Mul not implemented for T::Epsilon

So i have added 180 and compared to 180.

        fn relative_eq(&self, other: &Self, epsilon: Self::Epsilon, max_relative: Self::Epsilon) -> bool {
                let f180: T = flt(180.0);
                let diff: T = f180 + (self.to_degrees() - other.to_degrees()).into();
                T::relative_eq(&diff, &f180, epsilon, max_relative)
            }

The end result should be the same, some loss of presicison for values close to zero. Do you see any issues with this?

[sidred]

Should be good to go!!

[Ogeon]

Oh, nice! I'll take a look.

The end result should be the same, some loss of presicison for values close to zero. Do you see any issues with this?

I know too little about these things... The other alternative is to add the T::Epsilon: Float clause.

[Ogeon]

Wouldn't it be better to change this to self.$element.relative_eq(&other.$element, epsilon, max_relative)? That would let us have only one macro for implementation, instead of special casing hue based types.

[sidred]

done

[sidred]

These tests don't pass with either method. add 180 or multiply epsilon by 180

    #[test]
    fn check_hue_zero() {
        let a = RgbHue::from(0.00000333232);
        let b = RgbHue::from(0.00000849893);
        assert_relative_eq!(a, b);
        // assert_relative_eq!(0.00000333232, 0.00000849893);
    }
    #[test]
    fn check_hue_180() {
        let a = RgbHue::from(180.0);
        let b = RgbHue::from(179.99998474121093750000);
        assert_relative_eq!(a, b);
        // assert_relative_eq!(0.00000333232, 0.00000849893);
    }

Let me look into this further.

[sidred]

This passes. Forgot f32 tag

    #[test]
    fn check_hue_180() {
        let a = RgbHue::from(180.0_f32);
        let b = RgbHue::from(179.99998474121093750000_f32);
        assert_relative_eq!(a, b);
    }

I am not sure about multiplting ulps by 180, but we can leave it for now.

[Ogeon]

These things are still pretty much Terra incognita for me, but I think it looks fine, overall. Thank you!

@homu r+

[homu]

📌 Commit 3b952b3 has been approved by Ogeon

[homu]

⚡ Test exempted - status

[Ogeon]

Oh, we forgot to refer to #50, but it's ok.