Skip to content

Fix behaviour of + and - with rational infinity #16111

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Fix behaviour of + and - with rational infinity #16111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aced02e
Fix behaviour of rational infinities with + and -
TotalVerb May 9, 2016
61b602d
Merge branch 'master' into fix-rational-infinity
TotalVerb Feb 28, 2017
75bc087
Merge branch 'master' into fix-rational-infinity
TotalVerb Mar 1, 2017
110fe56
fix type instability
TotalVerb Mar 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions base/rational.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,13 @@ Rational(n::T, d::T) where T<:Integer = Rational{T}(n,d)
Rational(n::Integer, d::Integer) = Rational(promote(n,d)...)
Rational(n::Integer) = Rational(n,one(n))

function divgcd(x::Integer,y::Integer)
g = gcd(x,y)
div(x,g), div(y,g)
function divgcd(x::Integer, y::Integer)
if iszero(x) && iszero(y)
promote(x, y)
else
g = gcd(x, y)
promote(div(x, g), div(y, g))
end
Copy link
Member

@StefanKarpinski StefanKarpinski Sep 14, 2017

Choose a reason for hiding this comment

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

Would it make more sense to promote to a common type first:

divgcd(x::Integer, y::Integer) = divgcd(promote(x, y)...)

function divgcd(x::T, y::T) where T<:Integer
    if !iszero(x) || !iszero(y)
        g = gcd(x, y)
        x ÷= g
        y ÷= g
    end
    return x, y
end

Copy link
Contributor Author

@TotalVerb TotalVerb Sep 14, 2017

Choose a reason for hiding this comment

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

It's possibly intentional that things are done this way, since div has special cases for unsigned/signed combinations.

julia> @which 0x1 ÷ -1
div(x::Unsigned, y::Union{Int128, Int16, Int32, Int64, Int8}) in Base at int.jl:160

This behavior might require reviewing for desirability/consistency.

end

"""
Expand Down Expand Up @@ -237,8 +241,12 @@ for (op,chop) in ((:+,:checked_add), (:-,:checked_sub),
(:rem,:rem), (:mod,:mod))
@eval begin
function ($op)(x::Rational, y::Rational)
xd, yd = divgcd(x.den, y.den)
Rational(($chop)(checked_mul(x.num,yd), checked_mul(y.num,xd)), checked_mul(x.den,yd))
if x.den == y.den
Rational(($chop)(x.num, y.num), x.den)
else
xd, yd = divgcd(x.den, y.den)
Rational(($chop)(checked_mul(x.num,yd), checked_mul(y.num,xd)), checked_mul(x.den,yd))
end
Copy link
Member

@StefanKarpinski StefanKarpinski Sep 14, 2017

Choose a reason for hiding this comment

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

Likewise, maybe promote first, but then again, maybe that can be another PR.

end
end
end
Expand Down
22 changes: 17 additions & 5 deletions test/numbers.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1021,6 +1021,17 @@ f9085() = typemax(UInt64) != 2.0^64
@test (1//typemax(Int)) / (1//typemax(Int)) == 1
@test_throws OverflowError (1//2)^63

for op in (+, -, *, /, ÷, %)
for (xfl, xra) in ((Inf, 1//0), (-Inf, -1//0)),
(yfl, yra) in ((Inf, 1//0), (-Inf, -1//0))
if isnan(op(xfl, yfl))
@test_throws Union{DivideError, ArgumentError} op(xra, yra)
else
@test op(xra, yra) == op(xfl, yfl)
end
end
end

@test @inferred(rationalize(Int, 3.0, 0.0)) === 3//1
@test @inferred(rationalize(Int, 3.0, 0)) === 3//1
@test_throws ArgumentError rationalize(Int, big(3.0), -1.)
Expand Down Expand Up @@ -2813,19 +2824,20 @@ testmi(typemax(UInt32)-UInt32(1000):typemax(UInt32), map(UInt32, 1:100))
@test_throws MethodError 3 // 4.5im

# PR #16995
let types = (Base.BitInteger_types..., BigInt, Bool,
@testset "promotion of +, -, *, /, ^" begin
types = (Base.BitInteger_types..., BigInt, Bool,
Rational{Int}, Rational{BigInt},
Float16, Float32, Float64, BigFloat,
Complex{Int}, Complex{UInt}, Complex32, Complex64, Complex128)
for S in types
for op in (+, -)
@testset for S in types
@testset for op in (+, -)
T = @inferred Base.promote_op(op, S)
t = @inferred op(one(S))
@test T === typeof(t)
end

for R in types
for op in (+, -, *, /, ^)
@testset for R in types
@testset for op in (+, -, *, /, ^)
T = @inferred Base.promote_op(op, S, R)
t = @inferred op(one(S), one(R))
@test T === typeof(t)
Expand Down