amv-dev / yata

Yet Another Technical Analysis library [for Rust]
Apache License 2.0
334 stars 52 forks source link

fixed many clippy warning #1

Closed olexiyb closed 4 years ago

olexiyb commented 4 years ago

the most important one is https://rust-lang.github.io/rust-clippy/master/index.html#float_cmp

amv-dev commented 4 years ago

Hello. Thanks for you PR.

It's not so obvious for me that we cannot perform strict equality checks of f64 in updated places. There are two cases when we're using strict equality checks of f64.

  1. When we need to find a value in a Window. Window just keeps a byte-to-byte copied values, so it is totally safe to perform strict equality check. The only thing when it might became a problem is if one (or both) of the comparable values is NaN. But there are other checks in the code for this situation. This case happens is SMM, Highest/Lowest family.
  2. When we want to prevent division by zero. According to your suggestions, there is a question for me: is there any case when a != b, but a - b == 0.0? If yes, then your suggestions are right. If no, then there is no reason for additional checks throw f64::EPSILON.
olexiyb commented 4 years ago

@amv-dev there are hundreds of articles about float comparison. From my experience, I can tell that you can never guarantee that 2 float numbers are identical as the calculation can be different for different processors and OS. And we used decimal type instead of float for all finance calculations.

Take a look here: https://floating-point-gui.de/errors/comparison/

Read about decimal https://en.wikipedia.org/wiki/Decimal_floating_point

amv-dev commented 4 years ago

The point is that you can't trust floats equality only when you performed any changing operations over the values before. In the current case we do not perform any calculations over the values before comparison. We just copy value from one place to another and then comparing 8(or 4) bytes with the other 8(or 4) bytes from that places. The key is that values are not changed between copy and comparison. It might be clear if you would think about the values not like f64, but like binary [8; u8].

Also the answer to my second question is no. There is no way that a != b, but a-b == 0.0. It can be simply checked by this code:

for i in 0..(u32::MAX-1) {
    let v1 = f32::from_bits(i);
    let v2 = f32::from_bits(i+1);

    if v1-v2 == 0.0 {
        panic!("{} - {} is zero!", v1, v2);
    }

    if v2-v1 == 0.0 {
        panic!("{} - {} is zero!", v1, v2);
    }
}

The only problem here is that

let i = 123u32; // some random number
let v1 = f32::from_bits(i);
let v2 = f32::from_bits(i+1);

let division = 1.0/(v1-v2); // infinity may occur here

But there is no way you can guarantee that aftert the division there will be no infinity, even throw checking with f64::EPSILON.

amv-dev commented 4 years ago

According to your suggestions I will change every float equality checking like this:

let a = 123f64;
let b = a.clone();

if a == b { // old version
}

if a.to_bits() == b.to_bits() { // new version
}

I think this will be more idiomatic and clear for the others. Also I will add additional comments to this.

olexiyb commented 4 years ago

@amv-dev I did review smm and highest_lowest algos and agree we can use pure comparison. I have reverted back original logic. I still use absdiff* macros in 4 places and unit tests. image

Let me know if any other issues in my pull request

amv-dev commented 4 years ago

@olexiyb, I tried to correctly apply your PR, but I think it will be better if you split it at 3 different PRs:

  1. All the typo fixes. I really want apply this part ASAP.
  2. All the regular clippy lint fixes that are not concern on calculation algorithms (f.e. tabs to spaces, code parts movement, if-else refactoring etc...)
  3. All the other sensitive stuff, that may act on performance and/or correctness of the calculations, include working with float values. This is the most controversial part of your PR for me right now.

Can you do this? I will really appreciate it.

olexiyb commented 4 years ago

1) and 2) done. Please review and merge these 2 pull requests