statrs-dev / statrs

Statistical computation library for Rust
https://docs.rs/statrs/latest/statrs/
MIT License
603 stars 84 forks source link

Replace `is_zero(x)` with `x == 0.0` #256

Closed FreezyLemon closed 3 months ago

FreezyLemon commented 3 months ago
fn is_zero(x: f64) -> bool {
    ulps_eq!(x, 0.0, max_ulps = 0)
}

turns into

::approx::Ulps::default().max_ulps(0).eq(x, 0.0)

which turns into

if (x - 0.0).abs() <= f64::EPSILON {
  return true;
}

if x.signum() != 0.0.signum() {
  return false;
}

x.to_bits() - 0.0.to_bits() == 0

The actual code is not quite as clear as this, it took me a few minutes to find the relevant places and shorten it to this.

is_zero was introduced in order to silence clippy warnings/errors and was a 1:1 replacement for x == 0.0.

I would argue that is_zero is unnecessarily complicated, not transparent and possibly incorrect (one example: 0.0 == -0.0 evaluates to true while is_zero(-0.0) will evaluate to false) and was not the right solution to the clippy warnings in the first place.

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 89.49%. Comparing base (44ccdb9) to head (180b3fa). Report is 8 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #256 +/- ## ========================================== - Coverage 89.49% 89.49% -0.01% ========================================== Files 50 49 -1 Lines 10851 10848 -3 ========================================== - Hits 9711 9708 -3 Misses 1140 1140 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

YeungOnion commented 3 months ago

The purpose it was added seems clear and no longer relevant. This means all cases of negative zero will likely be handled as if they were positive zero since we seem to only use

instead of checking sign bit.

Is adding some negative 0 test cases needed here? It would ensure that behavior that checks the sign bit is consistent. Or is there an alternative way to remember and enforce this decision against checking sign bit for zero values?

FreezyLemon commented 3 months ago

Hm.. on revisit, I think I missed something fairly trivial, don't ask me how.

is_zero(-0.0) does return true, not false like I said above. I came to a wrong conclusion while I was looking at the code and didn't test it afterwards. Sorry for that.

Anyways,

Is adding some negative 0 test cases needed here?

I think that my mistake above changes the context a bit. It's probably not necessary to test this...?

Or is there an alternative way to remember and enforce this decision against checking sign bit for zero values?

How often does statrs use bitwise operators or otherwise check the contents of specific bits of numbers? AFAICT, it's usually just "normal" number operators/functions. I guess some dependencies do this (as seen in the ulps_eq macro), but it should probably be assumed these are correct, right?