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

Jump to bottom

Fix ULP matcher #2152

Closed
wants to merge 6 commits into from
Closed

Fix ULP matcher #2152

wants to merge 6 commits into from

Conversation

yyny
Copy link

@yyny yyny commented Jan 13, 2021

In the ULP matcher implemented, there is a check (lc < 0) != (rc < 0) seemingly there to allow +0 and -0 to compare equal even if maxUlpDiff is zero. However, this check passes for any two numbers that do not have the same sign. This is incorrect, for example almostEqualUlps(-EPSILON, EPSILON, INT64_MAX) returns false, even though it is obviously true (2 ULPS < INT_MAX ULPS). I've reimplemented the matcher, using boost::math as a reference.

horenmar added a commit that referenced this pull request Jul 26, 2021
@horenmar
Fix ulp distance calculation for numbers with different signs
d0a9d87 
This is a simplification of the fix proposed in #2152, with the
critical function split out so that it can be tested directly,
without having to go through the ULP matcher.

Closes #2152
horenmar added a commit that referenced this pull request Jul 27, 2021
@horenmar
Fix ulp distance calculation for numbers with different signs
4836742 
This is a simplification of the fix proposed in #2152, with the
critical function split out so that it can be tested directly,
without having to go through the ULP matcher.

Closes #2152
horenmar added a commit that referenced this pull request Jul 27, 2021
@horenmar
Fix ulp distance calculation for numbers with different signs
bc74fef 
This is a simplification of the fix proposed in #2152, with the
critical function split out so that it can be tested directly,
without having to go through the ULP matcher.

Closes #2152
@horenmar horenmar closed this in 3d1cf95 Jul 27, 2021
@horenmar
Copy link
Member

Thanks for bringing it to my attention, I redid the fix to simplify the implementation and get rid of superfluous changes in the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
BugFix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@yyny @horenmar