-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
gh-71385: add relative delta parameter for assertAlmostEqual and assertNotAlmostEqual #96881
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
base: main
Are you sure you want to change the base?
Conversation
|
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
Lib/unittest/case.py
Outdated
|
|
||
| 3) Comparing that the relative difference between the two | ||
| objects is more than the given rel_delta. The relative | ||
| difference is applied with respect to the first object, |
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.
Is there any good reason to apply the difference with respect only to first object?
math.isclose() is symmetrical and I think having different behavior here would be potentially confusing.
Lib/unittest/case.py
Outdated
| @@ -911,6 +919,15 @@ def assertAlmostEqual(self, first, second, places=None, msg=None, | |||
| safe_repr(second), | |||
| safe_repr(delta), | |||
| safe_repr(diff)) | |||
| elif rel_delta is not None: | |||
| if diff < rel_delta*abs(first): | |||
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.
| if diff < rel_delta*abs(first): | |
| if math.isclose(first,second,rel_tol=rel_delta, abs_tol=0.0): |
import math is necessary at the beginning of the file if you want to inlcude this suggestion
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.
Some small nits :)
Misc/NEWS.d/next/Library/2022-09-16-21-51-41.gh-issue-71385.r5XUzt.rst
Outdated
Show resolved
Hide resolved
| first_dt = datetime.timedelta(seconds=20) | ||
| second_dt = datetime.timedelta(seconds=15) | ||
|
|
||
| self.assertAlmostEqual(first_dt, second_dt, rel_delta=0.5) |
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.
Let's add two datetime objects together with timedelta as rel_delta
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.
From my point of view this seems not well defined. The clearest method to see are the units: Within the calculation, we multiply the relative delta with one of the objects: timedelta * datetime would have a unit of square seconds ...
|
@wehlgrundspitze your PR title should be: |
Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
….TestCase.assertAlmostEqual
|
Thanks for your comments and feedback. I implemented them all and hope for acceptance. |
|
It seems to me that both existing and modified code don't have defined behavior when one (or both) input are either float('nan') or float('inf'). Especially assertNotAlmostEqual will fail when both of it's arguments are NaNs. This is different than math.isclose() and potentially confusing. https://docs.python.org/3/library/math.html#math.isclose Either we fix it or at least add a note in docstring. |
… to handle +-inf and NaN according to IEEE 754 standard
…ssert(Not)AlmostEqual
|
That was a great remark. I fixed the algorithm to handle +-inf and NaN according to IEEE 754 (and therefore consistent with math.isclose) and wrote an additional test about it. |


relative delta keyword for assertAlmostEqual and assertNotAlmostEqual #71385
gh-71385: a keyword "rel_delta" is introduced to the functions assertAlmostEqual and assertNotAlmostEqual to allow comparisons based on the relative difference of two values
more detailed history in the corresponding issue #71385