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

Rational multiplication 0 * Inf results in DivideError #39638

Open
mbravenboer opened this issue Feb 12, 2021 · 5 comments
Open

Rational multiplication 0 * Inf results in DivideError #39638

mbravenboer opened this issue Feb 12, 2021 · 5 comments
Labels
error messages Better, more actionable error messages rationals The Rational type and values thereof

Comments

@mbravenboer
Copy link

julia> x = Rational{Int128}(0,1)
0//1

julia> y = Rational{Int128}(1,0)
1//0

julia> x == 0
true

julia> y == Inf
true

julia> x * y
ERROR: DivideError: integer division error
Stacktrace:
 [1] div at ./int.jl:909 [inlined]
 [2] divgcd at ./rational.jl:44 [inlined]
 [3] *(::Rational{Int128}, ::Rational{Int128}) at ./rational.jl:308
 [4] top-level scope at REPL[26]:1

The problem is that the gcd is 0 with x.num and y.den both being 0

function *(x::Rational, y::Rational)
    xn, yd = divgcd(promote(x.num, y.den)...)
    xd, yn = divgcd(promote(x.den, y.num)...)
    unsafe_rational(checked_mul(xn, yn), checked_mul(xd, yd))
end

function divgcd(x::Integer,y::Integer)
    g = gcd(x,y)
    div(x,g), div(y,g)
end

For any x, I think an appropriate value for 0 * x would be 0?

@simeonschaub
Copy link
Member

This is definitely intentional, although it might deserve a better error message. Note that the corresponding floating point computation 0.0 * Inf also returns NaN, not 0.0. We could technically allow 0 // 0 as the equivalent of NaN for Rational, but silent NaNs often cause more problems than they solve, so I don't really think we want to do this.

@simeonschaub simeonschaub added rationals The Rational type and values thereof error messages Better, more actionable error messages labels Feb 12, 2021
@mbravenboer
Copy link
Author

mbravenboer commented Feb 12, 2021

Thanks for the background @simeonschaub .

We were generally hoping that the Rational type would have more of the desirable mathematical properties, like associativity, 0 * x = x etc (modulo overflow). If that is a goal of the Rational type (maybe it is not), then the binary floating-point standard is maybe not a good reference?

@simeonschaub
Copy link
Member

That's generally not something that's possible in a consistent way if you decide to include +/- infinity as well. In practice, I would think that you can often just ignore the existence of these infinities or explicitly check against them.

@timholy
Copy link
Member

timholy commented Feb 12, 2021

0 * Inf = 0 definitely seems like bad math. Consider $\lim_{x\rightarrow 0} x/x$; the answer is of course 1, not zero. But of course $\lim_{x\rightarrow 0} 2x/x = 2$. So it's not an operation you can define meaningfully, so either the error or a NaN-equivalent is perfect.

@mbravenboer
Copy link
Author

I see, yeah I realize that in our system maybe the problem is that infinity should not have been allowed to begin with. The Rational type chooses to support infinity as a special value, but I probably didn't want that. The mathematical properties I had in mind seem okay if you exclude infinity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error messages Better, more actionable error messages rationals The Rational type and values thereof
Projects
None yet
Development

No branches or pull requests

3 participants