Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

relative_eq() has incorrect behavior for small values #65

Open
dsroche opened this issue Feb 1, 2021 · 3 comments
Open

relative_eq() has incorrect behavior for small values #65

dsroche opened this issue Feb 1, 2021 · 3 comments

Comments

@dsroche
Copy link

dsroche commented Feb 1, 2021

Consider this small example:

let diff: f32 = 1.0e-7;
let x: f32 = 1.0e-5;
let y = x - diff;
assert_ne!(x, y);
relative_eq!(x, y) // returns true, but should be false

Note that x and y are normal, full-precision floating-point values (not "subnormals"), above the epsilon threshold, and their relative error is (x - y)/x, which equals 0.01, far above the default threshold. However, they are marked as "relatively equal" in the current implementation.

The problem is in the logic of the provided implementation of RelativeEq::relative_eq() for f32 and f64, namely these lines:

let abs_diff = $T::abs(self - other);
// For when the numbers are really close together
if abs_diff <= epsilon {
return true;
}

While the precise definition of relative equality and the parameters epsilon and max_relative are never clearly specified in the documentation, based on the referenced blog posts, it seems that the intention is to use epsilon as a zero-closeness threshold, and max_relative as the relative error threshold.

The issue stems from not checking the two input numbers (self and other in this case) individually against epsilon for a zero-threshold, but instead testing their absolute difference against epsilon. As a result, relative equality works as expected for large numbers, but in practice defaults to absolute-difference equality for values less than 1.

@Stargateur
Copy link

Stargateur commented Mar 11, 2021

Rust follow IEEE 754 all is specified here. (but I have no idea if this will answer you)

@imjasonmiller
Copy link

imjasonmiller commented Mar 11, 2021

@dsroche does the code below illustrate your point? I'm not that familiar with relative floating point comparisons:

fn relative_eq(lhs: f32, rhs: f32, epsilon: f32, max_relative: f32) -> bool {
    // Handle same infinities
    if lhs == rhs {
        return true;
    }

    // Handle remaining infinities
    if f32::is_infinite(lhs) || f32::is_infinite(rhs) {
        return false;
    }

    let abs_diff = f32::abs(lhs - rhs);

    // For when the numbers are really close together
    if abs_diff <= epsilon {
        return true;
    }

    let abs_lhs = f32::abs(lhs);
    let abs_rhs = f32::abs(rhs);

    let largest = if abs_rhs > abs_lhs { abs_rhs } else { abs_lhs };

    println!("This part is never reached");
    // Use a relative difference comparison
    abs_diff <= largest * max_relative
}

fn main() {
    let diff = 1.0e-7;
    let x: f32 = 1.0e-5;
    let y: f32 = x - diff;

    println!(
        "relative_eq!(x, y) = {}",
        relative_eq(x, y, f32::EPSILON, f32::EPSILON) // true
    );

    println!(
        "abs_diff <= largest * max_relative = {}",
        (x - y).abs() <= 1.0e-5 * f32::EPSILON, // false
    );
}

So one has to pass their own epsilon in order for it to work for small values? E.g:

use approx::relative_eq;

relative_eq!(x, y, epsilon = 1.0e-8f32)

@brendanzab
Copy link
Owner

Oooh thanks for the report! I may have got something wrong in the implementation - it's been a long time since I looked inside. And to be honest floating point numerics are not something I'm an expert on. I'd be interested if we can find a fix for this though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants