Skip to content
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

Merged
merged 4 commits into from
Jun 3, 2024

Conversation

bradenmacdonald
Copy link
Contributor

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:

assertAlmostEquals(5e-7, 6e-7); // approx 20% different value
assertAlmostEquals(1e-8, 1e-9); // different order of magnitude

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 default tolerance 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.

@bradenmacdonald bradenmacdonald requested a review from kt3k as a code owner March 10, 2024 20:21
@CLAassistant
Copy link

CLAassistant commented Mar 10, 2024

CLA assistant check
All committers have signed the CLA.

@iuioiua iuioiua changed the title feat(assert): assertAlmostEquals() sets useful tolerance automatically feat(assert): assertAlmostEquals() sets useful tolerance automatically Mar 10, 2024
Copy link
Contributor

@iuioiua iuioiua left a 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.

@kt3k kt3k added the 1.0 label Mar 25, 2024
@kt3k
Copy link
Member

kt3k commented Mar 25, 2024

This change makes sense to me. However because this is a breaking change, we're planning to land this at v1.0

Copy link

codecov bot commented Apr 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.12%. Comparing base (b9374d3) to head (8fdc153).

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.
📢 Have feedback on the report? Share it here.

@kt3k kt3k changed the title feat(assert): assertAlmostEquals() sets useful tolerance automatically BREAKING(assert): assertAlmostEquals() sets useful tolerance automatically May 31, 2024
@iuioiua iuioiua merged commit 527b5d4 into denoland:main Jun 3, 2024
12 checks passed
@jsejcksn
Copy link
Contributor

jsejcksn commented Jun 6, 2024

one hundred thousandth of a percent of the expected value

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.

@kt3k
Copy link
Member

kt3k commented Jun 6, 2024

Sounds a good and interesting point. I searched a bit of the other standard libraries. Python has math.isclose in its standard library and it seems having the default of relative tolerance of 1e-9 (instead of 1e-7) https://docs.python.org/3/library/math.html#math.isclose

@bradenmacdonald
Copy link
Contributor Author

can you explain the motivation behind this default value?

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.

@bradenmacdonald bradenmacdonald deleted the assert-almost-equals-auto branch June 6, 2024 17:40
@iuioiua
Copy link
Contributor

iuioiua commented Jun 6, 2024

I agree - we should document the justification somewhere.

1e-7 "felt" like a value that would hide most tiny rounding errors while catching most math/computation/formula issues.

Even something similar to this might suffice as a justification.

@jsejcksn
Copy link
Contributor

jsejcksn commented Jun 7, 2024

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

@babiabeo

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants