-
-
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 bugs for x^Val and add tests #21226
Conversation
base/math.jl
Outdated
@inline ^{p}(x::Float16, ::Type{Val{p}}) = Float16(Float32(x) ^ Val{p}) | ||
@inline ^{p}(x::Float16, ::Type{Val{p}}) = Float16(x ^ p) | ||
@inline ^{p}(x::Float32, ::Type{Val{p}}) = Float32(x ^ p) | ||
@inline ^{p}(x::Float64, ::Type{Val{p}}) = Float64(x ^ p) | ||
|
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 used to work, but it was invalidated by #20889. It should have been changed to:
@inline literal_pow{p}(::typeof(^), x::Float16, ::Type{Val{p}}) = Float16(literal_pow(^,Float32(x),Val{p}))
We shouldn't be defining ^
methods for Val
at all any more.
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.
so these are also incorrect now then?
base/gmp.jl:447:^{p}(x::BigInt, ::Type{Val{p}}) = x^p
base/irrationals.jl:215:^{p}(::Irrational{:e}, ::Type{Val{p}}) = exp(p)
base/mpfr.jl:508:^{p}(x::BigFloat, ::Type{Val{p}}) = x^p
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 also just delete this method and use the fallback, since performance probably isn't really an issue for this method: the fallback Float16(Float32(x)^Float32(p))
should be fast enough.)
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.
@tkelman, yes, I commented on those below — those methods can now just be deleted. (The fallback literal_pow
will now do the right thing for those types.)
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.
Cheers, glad we pinged you on this! I'll update the PR.
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.
I've removed the methods I added and put in the literal_pow
one and get:
Test threw an exception of type MethodError
Expression: x ^ Val{1} === x
MethodError: no method matching ^(::Float32, ::Type{Val{1}})
Closest candidates are:
^(::Float32, !Matched::Float32) at math.jl:697
^(::Float32, !Matched::Integer) at math.jl:699
^(!Matched::BigInt, ::Type{Val{p}}) where p at gmp.jl:447
Do I need to add a line for the Float32
and Float64
types similar to 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.
The problem is with the tests; I added a comment explaining why.
There are also |
test/float16.jl
Outdated
@@ -61,6 +61,7 @@ g = Float16(1.) | |||
@test f*g === Float16(2f0) | |||
@test f/g === Float16(2f0) | |||
@test f^g === Float16(2f0) | |||
@test f^Val{1} === Float16(2f0) |
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 test (and the similar tests below) should be removed, since there are (or should be) no Val
methods for ^
.
To test literal_pow
, just @test f^1 === Float16(2f0)
, since f^1
gets converted into a literal_pow
call.
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.
Thanks!
The
Float16
method was trying to fall back to something that doesn't exist...