Skip to content

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
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

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.

@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.

@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