-
-
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
rational powers (fixes #18114) #18118
Conversation
@@ -2137,7 +2137,8 @@ rationalize(nextfloat(0.0)) == 0//1 | |||
# rational-exponent promotion rules (issue #3155): | |||
@test 2.0f0^(1//3) == 2.0f0^(1.0f0/3) | |||
@test 2^(1//3) == 2^(1/3) | |||
|
|||
# no loss of precision for rational powers (issue #18114) | |||
@test_approx_eq_eps BigFloat(2)^(BigFloat(1)/BigFloat(3)) BigFloat(2)^(1//3) 1e-20 |
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.
Cleaner to do @test BigFloat(2)^(BigFloat(1)/BigFloat(3)) ≈ BigFloat(2)^(1//3)
... this will test to sqrt(eps) precision by default, about 4e-39 for the default eps(BigFloat)
.
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 think even ==
should work in this case.
^{T<:AbstractFloat}(x::T, y::Rational) = x^(convert(T, y.num / y.den)) | ||
^{T<:AbstractFloat}(x::Complex{T}, y::Rational) = x^(convert(T, y.num / y.den)) | ||
^{T<:AbstractFloat}(x::T, y::Rational) = x^(convert(T,y.num)/convert(T,y.den)) | ||
^{T<:AbstractFloat}(x::Complex{T}, y::Rational) = x^(convert(T,y.num)/convert(T,y.den)) |
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.
You could probably simplify these to x^convert(T,y)
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.
My thinking here was that there is a higher chance to have a conversion from Int
then from Rational
. But if you think it's ok to have x^convert(T,y)
I can change it.
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.
convert(T,y)
is safe, because there is a generic conversion from Rational to AbstractFloat.
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.
It will dispatch to this method:
Lines 71 to 74 in b506041
function convert{T<:AbstractFloat,S}(::Type{T}, x::Rational{S}) | |
P = promote_type(T,S) | |
convert(T, convert(P,x.num)/convert(P,x.den)) | |
end |
which actually will be better, as it will handle, e.g.
^(x::Float64, y::Rational{BigInt})
by promoting to BigFloat
.
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.
Maybe I don't see the whole picture, but why is there even a specialization for AbstractFloat
at all? Can't this case be treated by ^(x::Number, y::Number) = ^(promote(x,y)...)
?
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.
No, because then it will call ^(x::Number, y::Rational)
instead. (Remember, Julia always calls the most specific method.)
Simplified the definition of `^`
Thanks! |
Fixes precision problem for rational powers of BigFloats (JuliaLang#18114)
Fixes #18114