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

Incorrect results from isapprox with Integer args #55814

Closed
yurivish opened this issue Sep 19, 2024 · 3 comments
Closed

Incorrect results from isapprox with Integer args #55814

yurivish opened this issue Sep 19, 2024 · 3 comments
Labels
correctness bug ⚠ Bugs that are likely to lead to incorrect results in user code without throwing duplicate Indicates similar issues or pull requests maths Mathematical functions

Comments

@yurivish
Copy link
Contributor

yurivish commented Sep 19, 2024

I noticed the following results from isapprox:

julia> isapprox(1, 2, atol=1)
true

julia> isapprox(UInt(1), UInt(2), atol=1)
false

This function is documented to return true if the distance between the two inputs is within tolerance bounds, yet it returns false in the second case despite the fact that the two numbers are distance 1 apart. In fact, it will return false even if the tolerance is increased:

julia> isapprox(UInt(1), UInt(2), atol=100)
false

julia> isapprox(UInt(1), UInt(2), atol=1_000_000)
false

The same behavior occurs for relative tolerances:

julia> isapprox(1, 2, rtol=1)
true

julia> isapprox(UInt(1), UInt(2), rtol=1)
false

julia> isapprox(UInt(1), UInt(2), rtol=100)
false

julia> isapprox(UInt(1), UInt(2), rtol=1_000_000)
false

Similar behavior can be seen for signed integers, where eg. 125 is considered to be within an absolute distance of 10 from -125:

julia> isapprox(Int8(125), Int8(-125), atol=10)
true

julia> isapprox(Int8(-125), Int8(125), atol=10)
true

This appears to be due to an integer overflow bug in the isapprox implementation for integer arguments. The generic implementation has a comment about avoiding integer overflow but no such accounting is done in the version specialized for integers.

@yurivish yurivish changed the title Incorrect results from isapprox Possibly incorrect results from isapprox Sep 19, 2024
@mbauman mbauman changed the title Possibly incorrect results from isapprox Incorrect results from isapprox with Unsigned args Sep 19, 2024
@yurivish yurivish changed the title Incorrect results from isapprox with Unsigned args Incorrect results from isapprox with Integer args Sep 19, 2024
@giordano
Copy link
Contributor

giordano commented Sep 19, 2024

This

diff --git a/base/floatfuncs.jl b/base/floatfuncs.jl
index 67e7899b410..ac482faafc2 100644
--- a/base/floatfuncs.jl
+++ b/base/floatfuncs.jl
@@ -226,13 +226,14 @@ function isapprox(x::Number, y::Number;
          (nans && isnan(x) && isnan(y))
 end
 
-function isapprox(x::Integer, y::Integer;
-                  atol::Real=0, rtol::Real=rtoldefault(x,y,atol),
+function isapprox(_x::Integer, _y::Integer;
+                  atol::Real=0, rtol::Real=rtoldefault(_x, _y, atol),
                   nans::Bool=false, norm::Function=abs)
+    x, y = minmax(_x, _y)
     if norm === abs && atol < 1 && rtol == 0
         return x == y
     else
-        return norm(x - y) <= max(atol, rtol*max(norm(x), norm(y)))
+        return norm(y - x) <= max(atol, rtol*max(norm(x), norm(y)))
     end
 end

should be enough to fix the order-dependent cases, right? Then the other problem is the overflowing of y - x, which can be caught by checking whether the difference is negative, but I don't know how to proceed after that, widening the two numbers?

@nsajko
Copy link
Contributor

nsajko commented Sep 19, 2024

It could make sense to simply convert to AbstractFloat. But that would cause breakage in other places, e.g., for integers not representable as Float64.

@nsajko nsajko added the maths Mathematical functions label Sep 19, 2024
@nsajko
Copy link
Contributor

nsajko commented Sep 19, 2024

Duplicate of #50380

@nsajko nsajko closed this as not planned Won't fix, can't repro, duplicate, stale Sep 19, 2024
@nsajko nsajko added duplicate Indicates similar issues or pull requests correctness bug ⚠ Bugs that are likely to lead to incorrect results in user code without throwing labels Sep 19, 2024
@nsajko nsajko marked this as a duplicate of #50380 Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
correctness bug ⚠ Bugs that are likely to lead to incorrect results in user code without throwing duplicate Indicates similar issues or pull requests maths Mathematical functions
Projects
None yet
Development

No branches or pull requests

3 participants