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

[nhz2]

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]

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

[oscardssmith]

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

[nhz2]

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]

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]

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]

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

[nhz2]

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

[nhz2]

Bump

[nhz2]

This changes the behavior of some edge cases, so it might be good to run pkgeval.

[nhz2]

@nanosoldier runtests()

[nanosoldier]

The package evaluation job you requested has completed - possible new issues were detected. The full report is available.

Report summary

❗ Packages that crashed

2 packages crashed on the previous version too.

✖ Packages that failed

18 packages failed only on the current version.

• Package has test failures: 5 packages
• Package tests unexpectedly errored: 1 packages
• Tests became inactive: 2 packages
• Test duration exceeded the time limit: 10 packages

1569 packages failed on the previous version too.

✔ Packages that passed tests

12 packages passed tests only on the current version.

• Other: 12 packages

5223 packages passed tests on the previous version too.

~ Packages that at least loaded

6 packages successfully loaded only on the current version.

• Other: 6 packages

3216 packages successfully loaded on the previous version too.

➖ Packages that were skipped altogether

910 packages were skipped on the previous version too.

[nhz2]

@nanosoldier runtests(["SimpleLooper", "CharacteristicInvFourier", "Andes", "MetaGraphsNext", "CycleWalk", "AEMS", "PotentialFlow", "PlutoPages", "ILMPostProcessing", "DiffusionGarnet", "SBMLToolkit", "FlightSims", "GasChem", "PlantGeomTurtle", "StateSpaceAnalysis", "CollectiveSpins", "QuantumSavory", "SBMLToolkitTestSuite"])

[nanosoldier]

The package evaluation job you requested has completed - possible new issues were detected. The full report is available.

Report summary

✖ Packages that failed

1 packages failed only on the current version.

• Package tests unexpectedly errored: 1 packages

2 packages failed on the previous version too.

✔ Packages that passed tests

9 packages passed tests on the previous version too.

~ Packages that at least loaded

6 packages successfully loaded on the previous version too.

[nhz2]

@adienes, I think this is ready to be merged. pkgeval looks good