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

Avoid silent overflow in /(x::Rational, y::Complex{Int}) #53453

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

nhz2
Copy link
Contributor

@nhz2 nhz2 commented Feb 23, 2024

This change catches the overflow in

(Int8(1)//Int8(1)) / (Int8(100) + Int8(100)im)

instead of incorrectly returning

25//8 - 25//8*im

This PR removes some of the specific / methods that involve complex rationals in rational.jl.
The default methods in complex.jl have better promotion rules and edge case checking. This includes the checks added in #32050, fixing #23134 for rational numbers.

The previous dispatch to // ignored overflow errors (see #53435), but would return the correct answer if the numerator was zero even if the denominator would overflow.

@nhz2 nhz2 added complex Complex numbers rationals The Rational type and values thereof labels Feb 23, 2024
@nhz2 nhz2 changed the title Avoid silent overflow in /(x::Rational, y::Complex{Integer}) Avoid silent overflow in /(x::Rational, y::Complex{Int}) Feb 23, 2024
@nhz2 nhz2 marked this pull request as draft February 26, 2024 18:04
@nhz2
Copy link
Contributor Author

nhz2 commented Feb 26, 2024

This implementation seems to cause extra allocations. I'll try something else.

@oscardssmith
Copy link
Member

Are you sure this is causing allocations? That would be pretty surprising.

@nhz2
Copy link
Contributor Author

nhz2 commented Feb 26, 2024

Here are some basic benchmarks:

@btime $(1//1)/$(complex(1,2))

PR: 109.592 ns (0 allocations: 0 bytes)
master: 41.488 ns (0 allocations: 0 bytes)

@btime $(big(1//1))/$(big(complex(1//1, 2//1)))

PR: 1.272 μs (65 allocations: 2.01 KiB)
master: 1.237 μs (65 allocations: 2.01 KiB)

@btime $(1//1)/$(complex(1//1, 2//1))

PR: 99.083 ns (0 allocations: 0 bytes)
master: 98.738 ns (0 allocations: 0 bytes)

@btime $(complex(1//1, 2//1))/$(1)

PR: 13.401 ns (0 allocations: 0 bytes)
master: 13.169 ns (0 allocations: 0 bytes)

@btime $(complex(1//1, 2//1))/$(1//1)

PR: 23.829 ns (0 allocations: 0 bytes)
master: 24.063 ns (0 allocations: 0 bytes)

@nhz2
Copy link
Contributor Author

nhz2 commented Feb 26, 2024

Are you sure this is causing allocations? That would be pretty surprising.

My previous attempt to fix this was, this version doesn't have that issue.

@nhz2
Copy link
Contributor Author

nhz2 commented Feb 26, 2024

Here is the old version of the function that was causing allocations.

julia> function bar(a::R, z::S) where {R<:Real, S<:Complex}
           T = promote_type(R,S)
           z_p = T(z)
           if z_p isa Complex{<:Rational}
               # avoid overflow if a is zero and z isn't zero
               if iszero(z_p)
                   throw(DivideError())
               elseif iszero(a)
                   return a*inv(oneunit(z_p))
               end
           end
           a*inv(z_p)
       end
bar (generic function with 1 method)

julia> using BenchmarkTools

julia> @btime bar($(1.0), $(2.0+im))
  108.677 ns (3 allocations: 128 bytes)
0.4 - 0.2im

@nhz2 nhz2 marked this pull request as ready for review February 26, 2024 22:33
@nhz2
Copy link
Contributor Author

nhz2 commented Jul 30, 2024

I removed the extra method for /(a::Rational, z::Complex{<:Integer}) because I don't think it was worth the added complexity.

@nhz2 nhz2 requested a review from nsajko October 19, 2024 11:50
base/rational.jl Outdated Show resolved Hide resolved
Co-authored-by: Neven Sajko <[email protected]>
@test (false//true) / complex(true//false, false//true) === 0//1 + 0//1*im
@test (true//true) / complex(true//true, true//false) === 0//1 + 0//1*im
@test_throws DivideError (0//1) / complex(0, 0)
@test_throws DivideError (1//1) / complex(0, 0)
Copy link
Contributor

@nsajko nsajko Oct 19, 2024

Choose a reason for hiding this comment

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

This is wrong. The correct result is positive infinity, just like here:

julia> (1 // 1) / 0
1//0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With complex numbers, there are many possible infinities, I'm not sure what infinity is correct.

julia> 1.0/(0.0im)
NaN + NaN*im

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, sorry.

@nhz2
Copy link
Contributor Author

nhz2 commented Nov 7, 2024

This PR is ready to merge and will help with testing #56478

@nhz2
Copy link
Contributor Author

nhz2 commented Nov 25, 2024

Bump

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

Successfully merging this pull request may close these issues.

4 participants