-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Avoid silent overflow in /(x::Rational, y::Complex{Int})
#53453
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
Conversation
/(x::Rational, y::Complex{Integer})/(x::Rational, y::Complex{Int})
|
|
|
Are you sure this is causing allocations? That would be pretty surprising. |
|
Here are some basic benchmarks: @btime $(1//1)/$(complex(1,2))PR: 109.592 ns (0 allocations: 0 bytes) @btime $(big(1//1))/$(big(complex(1//1, 2//1)))PR: 1.272 μs (65 allocations: 2.01 KiB) @btime $(1//1)/$(complex(1//1, 2//1))PR: 99.083 ns (0 allocations: 0 bytes) @btime $(complex(1//1, 2//1))/$(1)PR: 13.401 ns (0 allocations: 0 bytes) @btime $(complex(1//1, 2//1))/$(1//1)PR: 23.829 ns (0 allocations: 0 bytes) |
My previous attempt to fix this was, this version doesn't have that issue. |
|
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 |
|
I removed the extra method for |
Co-authored-by: Neven Sajko <s@purelymail.com>
|
This PR is ready to merge and will help with testing #56478 |
|
Bump |
|
This changes the behavior of some edge cases, so it might be good to run pkgeval. |
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. Report summary❗ Packages that crashed2 packages crashed on the previous version too. ✖ Packages that failed18 packages failed only on the current version.
1569 packages failed on the previous version too. ✔ Packages that passed tests12 packages passed tests only on the current version.
5223 packages passed tests on the previous version too. ~ Packages that at least loaded6 packages successfully loaded only on the current version.
3216 packages successfully loaded on the previous version too. ➖ Packages that were skipped altogether910 packages were skipped on the previous version too. |
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. Report summary✖ Packages that failed1 packages failed only on the current version.
2 packages failed on the previous version too. ✔ Packages that passed tests9 packages passed tests on the previous version too. ~ Packages that at least loaded6 packages successfully loaded on the previous version too. |
|
@adienes, I think this is ready to be merged. pkgeval looks good |
…#53453) This change catches the overflow in ```julia (Int8(1)//Int8(1)) / (Int8(100) + Int8(100)im) ``` instead of incorrectly returning ```julia 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 JuliaLang#32050, fixing JuliaLang#23134 for rational numbers. The previous dispatch to `//` ignored overflow errors (see JuliaLang#53435), but would return the correct answer if the numerator was zero even if the denominator would overflow. --------- Co-authored-by: Neven Sajko <s@purelymail.com>
This change catches the overflow in
instead of incorrectly returning
This PR removes some of the specific
/methods that involve complex rationals inrational.jl.The default methods in
complex.jlhave 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.