RFC: Calculations producing invalid rationals throw ArgumentError
#25702
Conversation
|
A few observations.
julia> 0//0
ERROR: ArgumentError: invalid rational: zero(Int64)//zero(Int64)
Stacktrace:
[1] Type at ./rational.jl:13 [inlined]
[2] Type at ./rational.jl:18 [inlined]
[3] //(::Int64, ::Int64) at ./rational.jl:40
[4] top-level scope
julia> (0//1)//0
ERROR: DivideError: integer division error
Stacktrace:
[1] div at ./int.jl:229 [inlined]
[2] divgcd(::Int64, ::Int64) at ./rational.jl:24
[3] //(::Rational{Int64}, ::Int64) at ./rational.jl:43
[4] top-level scope
|
|
Looks like somehow this change makes |
|
I'm unsure where the the issue is coming from. Will probably not have time to investigate for a little while. |
|
The fix for that is probably to delete |
Previously a calculation such as `(0//1) / 0` would result in a `DivideError` which does not adequately inform the user that `0//0` is not a constructable rational. Now an `ArgumentError` occurs which states "invalid rational: zero(Int)//zero(Int)". This change should allow errors from more complicated operations like `std([1//1])` be easier to understand.
omus
force-pushed
the
cv/invalid-rational
branch
from
1b91f72 to
ec18ebd
Compare
|
The problem ended up being me not returning the correct type when the |
|
Adding the triage label to ensure that this is ok to merge. |
| g = gcd(x, y) | ||
| if iszero(g) | ||
| Tx, Ty, Tg = typeof(x), typeof(y), typeof(g) | ||
| return zero(promote_op(div, Tx, Tg)), zero(promote_op(div, Ty, Tg)) |
There was a problem hiding this comment.
I'd think zero(div(x, one(g))) should work here.
There was a problem hiding this comment.
Greatest common (positive) divisor (or zero if x and y are both zero).
|
I'm not totally convinced this needs to be changed. Another option is to simplify things even further by removing the special |
Fixes #51731 and #51730 Also fixes one of the previously broken tests: ```julia @test_broken Rational{Int64}(UInt(1), typemin(Int32)) == Int64(1) // Int64(typemin(Int32)) ``` This PR ensures the `Rational{T}` constructor with concrete `T` will only throw if the final numerator and denominator cannot be represented by `T`, or are both zero. If the `T` in `Rational{T}` is not concrete, this PR tries to ensure the numerator and denominator are promoted to the same type. This means `-1*Rational{Integer}(-1, 0x01) == 1` doesn't throw now. A side effect of this is that `Rational{Integer}(Int8(-1), 0x01)` now throws. Also, related to <#25702 (comment)>, now `divgcd` doesn't change the types of its inputs and can handle `typemin`, but it still throws a `DivideError` if both inputs are zero.
Previously a calculation such as
(0//1) / 0would result in aDivideErrorwhich does not adequately inform the user that0//0is not a constructible rational. Now anArgumentErroroccurs which states "invalid rational: zero(Int)//zero(Int)".This change should allow errors from more complicated operations like
std([1//1])be slightly easier to understand.Before PR:
After PR:
Related to: #25300