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

diff: support for inf/-inf #150

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

eric-bonfadini
Copy link

Closes #146

@@ -264,10 +264,19 @@ def are_different(first, second, tolerance):
return False

first_is_nan, second_is_nan = bool(first != first), bool(second != second)
first_is_inf, second_is_inf = first == float('inf'), second == float('inf')
first_is_ninf = first == float('-inf')
second_is_ninf = second == float('-inf')

if first_is_nan or second_is_nan:
Copy link
Contributor

Choose a reason for hiding this comment

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

on a more mathematical level, should two nan or two inf or two -inf be considered the same? It is improbable that two inf are the same when you are running a diff. If we agree that they should be considered the same, then this all looks great!

Copy link
Author

Choose a reason for hiding this comment

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

I asked myself the same question while doing this PR and I found links like this stating that inf==inf and -inf==-inf .

On the contrary, it seems that they agree that Nan!=Nan , but that's outside the scope of this PR.

Let me know if you prefer not considering two inf the same, I can easily adapt tests and code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think following convention is preferred over mathematical precision, so this sounds good to me 👍

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

Successfully merging this pull request may close these issues.

Float comparator does not handle inf correctly
2 participants