-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix MatchNumberWithTolerance to better compare floating-point values #1145
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
Conversation
A good example of a place where the previous algorithm would have been less than ideal is: static void Main(string[] args)
{
int xi = 16777217;
int yi = 16777219;
int zi = yi - xi;
float xf = xi;
float yf = yi;
float zf = yf - xf;
// 16777219 - 16777217 = 2
Console.WriteLine($"{yi} - {xi} = {zi}");
// 16777220 - 16777216 = 4
Console.WriteLine($"{yf:G9} - {xf:G9} = {zf:G9}");
// 0x4B800002 - 0x4B800000 = 0x40800000
Console.WriteLine($"0x{BitConverter.SingleToInt32Bits(yf):X8} - 0x{BitConverter.SingleToInt32Bits(xf):X8} = 0x{BitConverter.SingleToInt32Bits(zf):X8}");
} As you can see, the inputs are The new algorithm rounds each input (both expected and actual) to a given number of significant digits (currently defaulting to |
@@ -23,7 +23,7 @@ namespace Microsoft.ML.Runtime.RunTests | |||
/// </summary> | |||
public abstract partial class BaseTestBaseline : BaseTestClass | |||
{ | |||
public const decimal Tolerance = 10_000_000; | |||
public const int DigitsOfPrecision = 7; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public [](start = 8, length = 6)
nit: can it be internal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, but the existing code already had the previous constant as public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it is used by inherited classes. And this is "just tests" so internal vs. public may not make that much of a difference.
In reply to: 222529405 [](ancestors = 222529405)
Thanks for the change, Tanner. Did you want to try enabling any tests with the PR, to see if it helps? In reply to: 426870598 [](ancestors = 426870598) |
Will this allow you to remove the tolerance related disables on #1008 before you merge that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of cases where this is failing // digitsOfPrecision = 1 , variance = 0.1 |
similarly for f1 = 0.7099695, f2 = 0.6931915 delta should be one but its value 0.02 but its value is 0.020000000000000018 |
@Anipik, you have to decide which rounding behavior is the most desirable as each has its pros/cons. I've defaulted to the IEEE default rounding mode as that tends to have the best overall behavior for the binary floating-point format. The algorithm should be generally sufficient for
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This updates
MatchNumberWithTolerance
to better compare floating-point values and enables it on Windows.The previous algorithm was not properly accounting for the distribution of binary floating-point values and would not allow a match for numbers that could have been reasonably considered as equivalent.