-
Notifications
You must be signed in to change notification settings - Fork 257
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
Special case testing floats to account for rounding/platform differences #615
Conversation
// If the attribute is called "floatTest" we know to compare | ||
// the values to within some delta. Otherwise, we can | ||
// do a straight lookup of the key | ||
let delta = 0.00000000001; |
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.
traditionally this is called epsilon :)
{"Expression":"log10e[]", "Value":"0.4342944819032518"}, | ||
{"Expression":"sqrt1/2[]", "Value":"0.7071067811865476"}, | ||
{"Expression":"sqrt2[]", "Value":"1.4142135623730951"} | ||
let delta = 0.00000000001; |
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.
thats really the only question about the epsilon(delta), ieee 32 gets you kind of 6-9 and ieee 64 gets you 15-17. just to complicate things intel machines use 80 internally. and truncate to 64 when you are actually saving to a gpr or memory...so...11 is pretty reasonable to assume 64 bit, but if you want to not assume that (and think js asserts that it must be 64), something like 5 would be more appropriate.
this is fine
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 seems all fine. i do think though that if we change from having float-test in .ts look for float tripes and have something that looks for booleans instead we could maybe hoist more baby functional unit tests directly into eve (strings, conditionals, aggregates etc)
The current tests ran into an issue comparing floats, where the generated value was different from the expected value by some rounding error. This was the case that was encountered:
To fix this, I changed the function that compares sets (
isSetEqual
) to special case float comparisons. If the attribute being compared is called "floatTest", then the comparison will succeed if the expected result and the actual result differ by less than some delta. The delta I chose was arbitrarily0.00000000001
, and we should discuss exactly what this delta should be.In implementing this, I faced some choices on how to special case the comparison. I arrived at my solution (using an attribute name as a flag) because
isSetEqual
executes deeply in the call stack, so any extra flags indicating a float comparison would have to be passed through several other functions. We can discuss if there is a more elegant solution here, as I'm not satisfied entirely with it.