-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fix hypot with multiple arguments #38158
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1107,8 +1107,68 @@ end | |
|
||
isdefined(Main, :Furlongs) || @eval Main include("testhelpers/Furlongs.jl") | ||
using .Main.Furlongs | ||
@test hypot(Furlong(0), Furlong(0)) == Furlong(0.0) | ||
@test hypot(Furlong(3), Furlong(4)) == Furlong(5.0) | ||
@test hypot(Complex(3), Complex(4)) === 5.0 | ||
@test hypot(Complex(6, 8), Complex(8, 6)) === 10.0*sqrt(2) | ||
@test (@inferred hypot(Furlong(0), Furlong(0))) == Furlong(0.0) | ||
@test (@inferred hypot(Furlong(3), Furlong(4))) == Furlong(5.0) | ||
@test (@inferred hypot(Furlong(NaN), Furlong(Inf))) == Furlong(Inf) | ||
@test (@inferred hypot(Furlong(Inf), Furlong(NaN))) == Furlong(Inf) | ||
@test (@inferred hypot(Furlong(0), Furlong(0), Furlong(0))) == Furlong(0.0) | ||
@test (@inferred hypot(Furlong(Inf), Furlong(Inf))) == Furlong(Inf) | ||
@test (@inferred hypot(Furlong(1), Furlong(1), Furlong(1))) == Furlong(sqrt(3)) | ||
@test (@inferred hypot(Furlong(Inf), Furlong(NaN), Furlong(0))) == Furlong(Inf) | ||
@test (@inferred hypot(Furlong(Inf), Furlong(Inf), Furlong(Inf))) == Furlong(Inf) | ||
@test isnan(hypot(Furlong(NaN), Furlong(0), Furlong(1))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can there be a test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is what it does currently (master and PR):
And I added a test for it also to make sure that continues to operate the same. Is that what you were intending? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for adding the test and for explaining that this PR doesn't change the error behavior of I still think it's the case that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like that also still fails, since we need to find the maximum value:
I can require them to have the same type via the method signature, but I'm more inclined to let the user call |
||
ex = @test_throws ErrorException hypot(Furlong(1), 1) | ||
@test startswith(ex.value.msg, "promotion of types ") | ||
|
||
@test_throws MethodError hypot() | ||
@test (@inferred hypot(floatmax())) == floatmax() | ||
@test (@inferred hypot(floatmax(), floatmax())) == Inf | ||
@test (@inferred hypot(floatmin(), floatmin())) == √2floatmin() | ||
@test (@inferred hypot(floatmin(), floatmin(), floatmin())) == √3floatmin() | ||
@test (@inferred hypot(1e-162)) ≈ 1e-162 | ||
@test (@inferred hypot(2e-162, 1e-162, 1e-162)) ≈ hypot(2, 1, 1)*1e-162 | ||
@test (@inferred hypot(1e162)) ≈ 1e162 | ||
@test hypot(-2) === 2.0 | ||
@test hypot(-2, 0) === 2.0 | ||
let i = typemax(Int) | ||
@test (@inferred hypot(i, i)) ≈ i * √2 | ||
@test (@inferred hypot(i, i, i)) ≈ i * √3 | ||
@test (@inferred hypot(i, i, i, i)) ≈ 2.0i | ||
@test (@inferred hypot(i//1, 1//i, 1//i)) ≈ i | ||
end | ||
let i = typemin(Int) | ||
@test (@inferred hypot(i, i)) ≈ -√2i | ||
@test (@inferred hypot(i, i, i)) ≈ -√3i | ||
@test (@inferred hypot(i, i, i, i)) ≈ -2.0i | ||
end | ||
@testset "$T" for T in (Float32, Float64) | ||
@test (@inferred hypot(T(Inf), T(NaN))) == T(Inf) # IEEE754 says so | ||
@test (@inferred hypot(T(Inf), T(3//2), T(NaN))) == T(Inf) | ||
@test (@inferred hypot(T(1e10), T(1e10), T(1e10), T(1e10))) ≈ 2e10 | ||
@test isnan_type(T, hypot(T(3), T(3//4), T(NaN))) | ||
@test hypot(T(1), T(0)) === T(1) | ||
@test hypot(T(1), T(0), T(0)) === T(1) | ||
@test (@inferred hypot(T(Inf), T(Inf), T(Inf))) == T(Inf) | ||
for s in (zero(T), floatmin(T)*1e3, floatmax(T)*1e-3, T(Inf)) | ||
@test hypot(1s, 2s) ≈ s * hypot(1, 2) rtol=8eps(T) | ||
@test hypot(1s, 2s, 3s) ≈ s * hypot(1, 2, 3) rtol=8eps(T) | ||
end | ||
end | ||
@testset "$T" for T in (Float16, Float32, Float64, BigFloat) | ||
let x = 1.1sqrt(floatmin(T)) | ||
@test (@inferred hypot(x, x/4)) ≈ x * sqrt(17/BigFloat(16)) | ||
@test (@inferred hypot(x, x/4, x/4)) ≈ x * sqrt(9/BigFloat(8)) | ||
end | ||
let x = 2sqrt(nextfloat(zero(T))) | ||
@test (@inferred hypot(x, x/4)) ≈ x * sqrt(17/BigFloat(16)) | ||
@test (@inferred hypot(x, x/4, x/4)) ≈ x * sqrt(9/BigFloat(8)) | ||
end | ||
let x = sqrt(nextfloat(zero(T))/eps(T))/8, f = sqrt(4eps(T)) | ||
@test hypot(x, x*f) ≈ x * hypot(one(f), f) rtol=eps(T) | ||
@test hypot(x, x*f, x*f) ≈ x * hypot(one(f), f, f) rtol=eps(T) | ||
end | ||
end | ||
# hypot on Complex returns Real | ||
@test (@inferred hypot(3, 4im)) === 5.0 | ||
@test (@inferred hypot(3, 4im, 12)) === 13.0 | ||
end |
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.
Shouldn't we still use this for
hypot
of `Float16?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.
We could, though as the PR was written, it would have been the wrong function to overload (since we want to always give it once chance to call
float
, then give it once chance to dispatch on the result, even if the type of the answer is the unchaged). It would be possible to define:But this also fall out of the generic definition
_hypot
.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.
This indeed seems to have broken things #38311 (comment)