Skip to content

Fix rationalize([T,] x::Rational; ...) #61057

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

Open
lvlte wants to merge 8 commits into
base: master
Choose a base branch
from
+29 −8
Open

Fix rationalize([T,] x::Rational; ...) #61057

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
82771ec
Fix `rationalize([T,] x::Rational; ...)`
lvlte Feb 17, 2026
bd0aa6f
Remove trailing blank line
lvlte Feb 17, 2026
13c4031
Merge branch 'master' into rationalize_rational_fix
lvlte Feb 18, 2026
a797f5a
Revert unnecessary changes
lvlte Feb 19, 2026
9dec955
Merge branch 'master' into rationalize_rational_fix
lvlte Apr 22, 2026
746b5c8
change default tol for rationals, throw if inexact with low tol
lvlte Apr 24, 2026
768dd9c
Merge branch 'master' into rationalize_rational_fix
lvlte May 3, 2026
2b0cfe2
Reset default tol, return `Rational{T}(x)` if `x` fits in `T`
lvlte May 3, 2026
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
23 changes: 15 additions & 8 deletions base/rational.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,11 +244,8 @@ julia> typeof(numerator(a))
BigInt
```
"""
function rationalize(::Type{T}, x::Union{AbstractFloat, Rational}, tol::Real) where T<:Integer
if tol < 0
throw(ArgumentError("negative tolerance $tol"))
end

function rationalize(::Type{T}, x::AbstractFloat, tol::Real) where T<:Integer
tol < 0 && throw(ArgumentError("Tolerance can not be negative. tol=$tol"))
T<:Unsigned && x < 0 && __throw_negate_unsigned()
isnan(x) && return T(x)//one(T)
isinf(x) && return unsafe_rational(x < 0 ? -one(T) : one(T), zero(T))
Expand All @@ -257,9 +254,8 @@ function rationalize(::Type{T}, x::Union{AbstractFloat, Rational}, tol::Real) wh
pp, qq = zero(T), one(T)

x = abs(x)
a = trunc(x)
r = x-a
y = one(x)
r, a = modf(x)
Comment thread
lvlte marked this conversation as resolved.
Outdated
tolx = oftype(x, tol)
nt, t, tt = tolx, zero(tolx), tolx
ia = np = nq = zero(T)
Expand Down Expand Up @@ -309,7 +305,18 @@ rationalize(x::Real; kvs...) = rationalize(Int, x; kvs...)
rationalize(::Type{T}, x::Complex; kvs...) where {T<:Integer} = Complex(rationalize(T, x.re; kvs...), rationalize(T, x.im; kvs...))
rationalize(x::Complex; kvs...) = Complex(rationalize(Int, x.re; kvs...), rationalize(Int, x.im; kvs...))
rationalize(::Type{T}, x::Rational; tol::Real = 0) where {T<:Integer} = rationalize(T, x, tol)
rationalize(x::Rational; kvs...) = x
rationalize(x::Rational{T}; kvs...) where {T<:Integer} = rationalize(T, x; kvs...)
function rationalize(::Type{T}, x::Rational, tol::Real) where {T<:Integer}
T<:Unsigned && x < 0 && __throw_negate_unsigned()
if 0 ≤ tol ≤ eps(float(x))
Copy link
Copy Markdown
Member

@adienes adienes Apr 22, 2026

Choose a reason for hiding this comment

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

technically I guess if isinf(float(x)) this might run into trouble, e.g.

julia> X = big"2"^100000000;

julia> using Base.MPFR; MPFR.set_emax!(Int64(100))
true

julia> float(X//1)
Inf

julia> eps(ans)
NaN

although I don't think MPFR.set_emax! is a public function so that's kind of a niche worry

Copy link
Copy Markdown
Contributor Author

@lvlte lvlte Apr 22, 2026

Choose a reason for hiding this comment

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

Maybe we can do :

    if 0 ≤ tol ≤ eps(float(x)) || isinf(float(x))
        try
            return Rational{T}(x)
        catch e
            isa(e,InexactError) || rethrow()
        end
    end
    return rationalize(T, float(x), tol)

If isinf(float(x)) try the conversion first and fallback with rationalize (which should return ±1//0) ?

try
return Rational{T}(x)
catch e
isa(e,InexactError) || rethrow()
Copy link
Copy Markdown
Member

@adienes adienes Apr 22, 2026

Choose a reason for hiding this comment

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

the InexactError might come from tol=0, but we want that to rethrow as well right? since float(x) may lose precision and the fallback won't end up throwing.

Copy link
Copy Markdown
Contributor Author

@lvlte lvlte Apr 22, 2026

Choose a reason for hiding this comment

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

I'm not sure, I did this to be consistent with the general behavior of rationalize, ie. when tol can't be satisfied it doesn't throw but return the best it can for T.

This brings us to #52335

Copy link
Copy Markdown
Contributor Author

@lvlte lvlte Apr 22, 2026

Choose a reason for hiding this comment

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

We can still do isa(e,InexactError) && tol != 0 || rethrow() since users using zero tol would most likely expect an error in this situation.

Copy link
Copy Markdown
Contributor Author

@lvlte lvlte Apr 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

In fact no.. the problem is that until now we have this behavior :

julia> r = 2616549525268589//9223372036854775807
2616549525268589//9223372036854775807

julia> Rational{Int32}(r)
ERROR: InexactError: trunc(Int32, 2616549525268589) [...]

julia> rationalize(Int32, r)    # <- default `tol` is zero
330434//1164784265              # <- loss of precision

And this is probably the expected behavior for most users calling rationalize without setting tol explicitly (the default zero tol for rationals is undocumented). That's why I suggest to enable "strict mode" via an additional parameter in a separate PR (which would handle this once for all implementations).

Copy link
Copy Markdown
Member

@adienes adienes Apr 23, 2026

Choose a reason for hiding this comment

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

Is it possible tol = 0 isn’t the right default in the first place? It might be tolerable (pun intended) to change that to 1/typemax(T) as a minor breaking change if that’s better ?

Copy link
Copy Markdown
Contributor Author

@lvlte lvlte Apr 23, 2026

Choose a reason for hiding this comment

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

I can tolerate that :) I totally agree, 1/typemax(T) for rationals is somehow the equivalent of eps(x) for floats. So we would have :

rationalize(::Type{T}, x::Rational; tol::Real = 1/typemax(T)) where {T<:Integer} = rationalize(T, x, tol)
rationalize(x::Rational{T}; kvs...) where {T<:Integer} = rationalize(T, x; kvs...)
function rationalize(::Type{T}, x::Rational, tol::Real) where {T<:Integer}
    T<:Unsigned && x < 0 && __throw_negate_unsigned()
    if 0 ≤ tol ≤ eps(float(x)) || isinf(float(x))
            return Rational{T}(x)
    end
    return rationalize(T, float(x), tol)
end

Ah and we would need a specific default tolerance for T=BigInt which doesn't have a typemax. Maybe we could leave zero for this one ?

Copy link
Copy Markdown
Member

@adienes adienes Apr 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

yeah, like Base.hastypemax(T) ? inv(typemax(T)) : zero(T)

Copy link
Copy Markdown
Contributor Author

@lvlte lvlte Apr 23, 2026

Choose a reason for hiding this comment

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

ok I will push that tomorrow

end
end
return rationalize(T, float(x), tol)
end
rationalize(x::Integer; kvs...) = Rational(x)
function rationalize(::Type{T}, x::Integer; kvs...) where {T<:Integer}
if Base.hastypemax(T) # BigInt doesn't
Expand Down
13 changes: 13 additions & 0 deletions test/rational.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -879,3 +879,16 @@ end
@test 1.0 != big(1//0)
@test Inf == big(1//0)
end

@testset "rationalize(Rational) (issue #60768)" begin
x = float(pi)
r = rationalize(x)
@test rationalize(Int32, r, tol=0.0) === Rational{Int32}(r) == r
@test rationalize(r) === rationalize(r, tol=0) === r
@test rationalize(r, tol=eps(float(r))) === r
@test rationalize(r, tol=0.1) == 16//5
for n=1:10
@test rationalize(r, tol=1/10^n) == rationalize(float(r), tol=1/10^n)
end
@test_throws OverflowError rationalize(UInt, -r)
end