Fix relative_eq for small values#102
Conversation
|
I am so sorry. I must have missed this PR completely. I am a bit concerned about all the tests that were rewritten in this PR. What is the reason behind that? |
The behavior changed, which required some tests to be modified. I also find the current implementation of the tests cleaner - there is less duplicate code. Some tests were written in such a way that they didn't actually test anything, for example because they tested the equality of identical numbers (implementation using == would also pass the test). |
| assert_relative_eq!(0.0f32, 1e-40f32, epsilon = 1e-40f32); | ||
| assert_relative_eq!(1e-40f32, 0.0f32, epsilon = 1e-40f32); | ||
| assert_relative_eq!(0.0f32, -1e-40f32, epsilon = 1e-40f32); | ||
| assert_relative_eq!(-1e-40f32, 0.0f32, epsilon = 1e-40f32); | ||
| assert_relative_eq!(0.0f32, 1e-20f32, epsilon = 1e-20f32); | ||
| assert_relative_eq!(1e-20f32, 0.0f32, epsilon = 1e-20f32); | ||
| assert_relative_eq!(0.0f32, -1e-20f32, epsilon = 1e-20f32); | ||
| assert_relative_eq!(-1e-20f32, 0.0f32, epsilon = 1e-20f32); | ||
|
|
||
| assert_relative_ne!(1e-40f32, 0.0f32, epsilon = 1e-41f32); | ||
| assert_relative_ne!(0.0f32, 1e-40f32, epsilon = 1e-41f32); | ||
| assert_relative_ne!(-1e-40f32, 0.0f32, epsilon = 1e-41f32); | ||
| assert_relative_ne!(0.0f32, -1e-40f32, epsilon = 1e-41f32); |
There was a problem hiding this comment.
This looks like a precision regression to me.
There was a problem hiding this comment.
1e-40 is denormalized number, it is not safe to compare denormalized numbers using relative comparison. Therefore they are marked equal by absolute comparison.
| } | ||
|
|
||
| fn compare_near_numbers(number: f32) { | ||
| let rel_eq_number = number * 1.0000001f32; |
There was a problem hiding this comment.
this multiplication has an internal error associated with it. Consider this little program:
fn main() {
let a = 1E-20;
let b = 1E-10;
println!("{}", a * b);
println!("{}", 1E-30);
}Output
0.0000000000000000000000000000009999999999999999
0.000000000000000000000000000001
How do we ensure that this multiplicative error does not invalidate our test (either via false positive or false negative).
There was a problem hiding this comment.
I dont understand.
-
How can you tell if the result is correct? We are talking about approximate comparisons, there is no strict border between correct and incorrect behavior.
-
Multiplication is defined by IEEE754 to return always correctly rounded result, so with given numbers it is just different way how to "hardcode" a number. In this case the result seems to be not correct but that is because input numbers are rounded.
1e-20 is stored as 9.9999999999999994515327145421e-21
1e-10 is stored as 1.0000000000000000364321973155e-10
Their product is most accurately represented as 9.99999999999999908174112566984e-31, not by 1e-30 which is stored as 1.00000000000000008333642060759E-30.
jonaspleyer
left a comment
jonaspleyer
left a comment
There was a problem hiding this comment.
I have left some comments which I would like to discuss.
I think that there is nothing wrong with testing cases which would be true via Your implementation of the tests is different since it involves a multiplication to calculate the numbers that should be compared. |
kitt159
force-pushed
the
relative_eq-small-values-fix
branch
from
1f4d408 to
bc35d0f
Compare
kitt159
force-pushed
the
relative_eq-small-values-fix
branch
from
bc35d0f to
2b9646e
Compare
I prefer to have nonredundant tests but I can keep most of the tests unchanged if you want. |
|
I have taken another longer look at your fix and I think that I will accept it into the codebase. However I will ask you to revert the changes done to the tests. I simply feel uncomfortable to change the test suite without any need. If you want, you can include issue #102 as a standalone test. Once the changes are included, I will create a new release candidate and notify some folds within the ecosystem to test the new version (most notable ndarray). As long as they do not find anything that interferes with their current implementations, I will wait a bit longer and ultimately release the new version. If you do not find the time to make the changes from above, simply let me know. I will then do them myself. Not a big deal. And thanks again for your continued effort in this! |
|
Can you confirm that you agree to license the contributions of this PR under the MIT license? We have an ongoing thread which is nearly resolved to add the MIT license to this crate to enable better compatibility with the Rust ecosystem: |
|
Another question that has bugged me is the following: your solution requires the additional implementation of the
|
2 participants
Change is described here
Fixes #65