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

Fix bugs for x^Val and add tests #21226

Merged
merged 1 commit into from
Apr 2, 2017
Merged

Fix bugs for x^Val and add tests #21226

merged 1 commit into from
Apr 2, 2017

Conversation

kshyatt
Copy link
Contributor

@kshyatt kshyatt commented Mar 29, 2017

The Float16 method was trying to fall back to something that doesn't exist...

@kshyatt kshyatt added maths Mathematical functions test This change adds or pertains to unit tests labels Mar 29, 2017
@tkelman
Copy link
Contributor

tkelman commented Mar 30, 2017

cc @stevengj ref 6c6bbba

@kshyatt kshyatt requested a review from stevengj March 30, 2017 16:17
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)

Copy link
Member

@stevengj stevengj Mar 30, 2017

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.

Copy link
Contributor

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

Copy link
Member

@stevengj stevengj Mar 30, 2017

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.)

Copy link
Member

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.)

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

@stevengj
Copy link
Member

There are also ^{p}(x::BigFloat, ::Type{Val{p}}) and ^{p}(::Irrational{:e}, ::Type{Val{p}}) methods that should be deleted, since they will now be handled by the literal_pow fallback.

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)
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@kshyatt kshyatt merged commit 1f4f4fb into master Apr 2, 2017
@kshyatt kshyatt deleted the ksh/valexp branch April 2, 2017 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maths Mathematical functions test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants