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

Add method to rationalize Rational #43427

Merged
merged 17 commits into from
Jun 14, 2023
Merged

Conversation

tantheta01
Copy link
Contributor

@tantheta01 tantheta01 commented Dec 15, 2021

Closes #43369.

@dkarrasch dkarrasch changed the title Address issue #43369 Add method to rationalize Rational Dec 15, 2021
@dkarrasch
Copy link
Member

@tantheta01 Thank you for your contribution and welcome to the community. I helped slightly by (i) making the title more descriptive, and (ii) rewording the OP to the effect that github associates this PR with the issue so when this PR is merged the issue is automatically closed.

@dkarrasch dkarrasch added the needs tests Unit tests are required for this change label Dec 15, 2021
@@ -216,8 +216,16 @@ function rationalize(::Type{T}, x::AbstractFloat, tol::Real) where T<:Integer
end
rationalize(::Type{T}, x::AbstractFloat; tol::Real = eps(x)) where {T<:Integer} = rationalize(T, x, tol)::Rational{T}
rationalize(x::AbstractFloat; kvs...) = rationalize(Int, x; kvs...)
rationalize(::Type{T}, x::Complex; kvs...) where {T<:Integer} = Complex(rationalize(T, x.re, kvs...)::Rational{T}, rationalize(T, x.im, kvs...)::Rational{T})
Copy link
Member

Choose a reason for hiding this comment

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

Oh wow, this looks like it was actually a bug. Thanks for catching that. Would be good to add a test for it.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like:

diff --git a/test/rational.jl b/test/rational.jl
--- test/rational.jl
+++ test/rational.jl
@@ -638,5 +638,11 @@
     @test rationalize(float(pi)im) == 0//1 + 165707065//52746197*im
     @test rationalize(Int8, float(pi)im) == 0//1 + 22//7*im
     @test rationalize(1.192 + 2.233im) == 149//125 + 2233//1000*im
     @test rationalize(Int8, 1.192 + 2.233im) == 118//99 + 67//30*im
+
+    # test: rationalize(x::Complex; kvs...)
+    precise_next = 7205759403792795//72057594037927936
+    @assert Float64(precise_next) == nextfloat(0.1)
+    @test rationalize(nextfloat(0.1) * im; tol=0) == precise_next * im
+    @test rationalize(0.1im; tol=eps(0.1)) == rationalize(0.1im)
 end

@@ -158,6 +158,7 @@ function rationalize(::Type{T}, x::AbstractFloat, tol::Real) where T<:Integer
if tol < 0
throw(ArgumentError("negative tolerance $tol"))
end

Copy link
Member

Choose a reason for hiding this comment

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

Better to avoid spurious whitespace changes like this.

base/rational.jl Outdated Show resolved Hide resolved
base/rational.jl Outdated Show resolved Hide resolved
base/rational.jl Outdated Show resolved Hide resolved
@oscardssmith oscardssmith added the rationals The Rational type and values thereof label Jun 8, 2023
@oscardssmith oscardssmith removed the needs tests Unit tests are required for this change label Jun 8, 2023
@oscardssmith
Copy link
Member

bumping this. I've added some more tests and I think it's ready to merge.

@oscardssmith
Copy link
Member

After an embarrassingly long time, this is finally passing tests. merging in a few days sans objections.

@oscardssmith oscardssmith merged commit 8a1b642 into JuliaLang:master Jun 14, 2023
@Keno
Copy link
Member

Keno commented Jun 14, 2023

This failed on CI and shouldn't have been merged. Please fix or revert in the morning.

Keno added a commit that referenced this pull request Jun 14, 2023
@KristofferC
Copy link
Member

Explicitly:

Error in testset rational:
--
Test Failed at C:\buildkite-agent\builds\win2k22-amdci6-4\julialang\julia-master\julia-d12ccd04fc\share\julia\test\rational.jl:739
Expression: rationalize(nextfloat(0.1) * im; tol = 0) == precise_next * im
Evaluated: 0//1 + 1//10*im == 0//1 + 7205759403792795//72057594037927936*im

@oscardssmith
Copy link
Member

Sorry about that, I saw that only windows was failing and assumed it was a platform problem. #50163 fixes it (there was an incorrect assumption in the test that Int is big enough to store the 53 bits of a Float64 mantissa).

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

Successfully merging this pull request may close these issues.

"rationalize" a rational
7 participants