-
Notifications
You must be signed in to change notification settings - Fork 632
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
BREAKING(assert): assertAlmostEquals()
sets useful tolerance automatically
#4460
BREAKING(assert): assertAlmostEquals()
sets useful tolerance automatically
#4460
Conversation
assertAlmostEquals()
sets useful tolerance automatically
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.
I very much like the reasoning and explanation behind this PR. Thank you. LGTM.
This change makes sense to me. However because this is a breaking change, we're planning to land this at v1.0 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4460 +/- ##
=======================================
Coverage 92.12% 92.12%
=======================================
Files 487 487
Lines 38771 38774 +3
Branches 5388 5391 +3
=======================================
+ Hits 35718 35721 +3
Misses 2997 2997
Partials 56 56 ☔ View full report in Codecov by Sentry. |
assertAlmostEquals()
sets useful tolerance automaticallyassertAlmostEquals()
sets useful tolerance automatically
I like the idea of improved ergonomics, but can you explain the motivation behind this default value? I think it's important to have documented justification for defaults instead of offering opaque and seemingly arbitrary values for consumers. |
Sounds a good and interesting point. I searched a bit of the other standard libraries. Python has |
1e-7 "felt" like a value that would hide most tiny rounding errors while catching most math/computation/formula issues. I don't really have any justification for what's ultimately an arbitrary default. Using 1e-9 for similarity to python would be fine too, but I think theirs is also just an arbitrary value - they don't provide any justification for it that I can see. |
I agree - we should document the justification somewhere.
Even something similar to this might suffice as a justification. |
Toward topical research — I found this quite insightful article which examines the complexities of comparing floating point numbers of varying magnitudes: Comparing Floating Point Numbers, 2012 Edition |
I was using
assertAlmostEquals
in my project and found that its behavior when dealing with small numbers is not very useful by default. For example, neither of the following will throw, despite having very different values:This definition of "almost equal" for numbers that have a different order of magnitude doesn't match my intuition about how such an assert should work.
If the goal of the assertion is to "take into account IEEE-754 double-precision floating-point representation limitations", then it is not doing that by default - you have to manually specify a precision based on the expected order of magnitude of the values that you're comparing. For projects that have to deal with a wide range of values, this is not ideal. Why don't we make the default "auto-detect" a useful precision range?
This proposal changes the behavior of
assertAlmostEquals
so that its defaulttolerance
is not an absolute numeric value, but is one hundred thousandth of a percent of the expected value. To make this change largely backwards compatible, you can still specify an absolute tolerance if you want to. But I think this makes the default version much more useful and avoids some footguns.All of the existing tests for this assert continue to pass unchanged, but the newly added tests only pass with this feature/fix in place. I'm not sure if this is considered a breaking change or not.