X Tutup
The Wayback Machine - https://web.archive.org/web/20250609224420/https://github.com/python/cpython/pull/96881
Skip to content

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

wehlgrundspitze
Copy link

@wehlgrundspitze wehlgrundspitze commented Sep 16, 2022

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

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@ghost
Copy link

ghost commented Sep 16, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@wehlgrundspitze wehlgrundspitze changed the title Fix issue 71385 Fix issue #71385 Sep 16, 2022

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,
Copy link

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.

@@ -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):
Copy link

@chalggg chalggg Sep 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small nits :)

first_dt = datetime.timedelta(seconds=20)
second_dt = datetime.timedelta(seconds=15)

self.assertAlmostEqual(first_dt, second_dt, rel_delta=0.5)
Copy link
Member

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

Copy link
Author

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

@sobolevn
Copy link
Member

@wehlgrundspitze your PR title should be: gh-71385: add relative delta parameter for assertAlmostEqual and assertNotAlmostEqual

@wehlgrundspitze wehlgrundspitze changed the title Fix issue #71385 gh-71385: add relative delta parameter for assertAlmostEqual and assertNotAlmostEqual Oct 8, 2022
@wehlgrundspitze
Copy link
Author

Thanks for your comments and feedback. I implemented them all and hope for acceptance.

@chalggg
Copy link

chalggg commented Oct 16, 2022

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.

@wehlgrundspitze
Copy link
Author

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.

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.

5 participants
X Tutup