Skip to content

Conversation

@nhz2
Copy link
Member

@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
Member 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
Member 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
Member 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
Member 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
Member 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
Co-authored-by: Neven Sajko <s@purelymail.com>
@nhz2
Copy link
Member Author

nhz2 commented Nov 7, 2024

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

@nhz2
Copy link
Member Author

nhz2 commented Nov 25, 2024

Bump

@nhz2 nhz2 marked this pull request as draft August 22, 2025 17:48
@nhz2 nhz2 marked this pull request as ready for review August 22, 2025 18:41
@nhz2 nhz2 added the needs pkgeval Tests for all registered packages should be run with this change label Aug 22, 2025
@nhz2
Copy link
Member Author

nhz2 commented Aug 22, 2025

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

@nhz2 nhz2 added the minor change Marginal behavior change acceptable for a minor release label Sep 13, 2025
@nhz2
Copy link
Member Author

nhz2 commented Sep 14, 2025

@nanosoldier runtests()

@nanosoldier
Copy link
Collaborator

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
Copy link
Member Author

nhz2 commented Sep 15, 2025

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

@nanosoldier
Copy link
Collaborator

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 nhz2 removed the needs pkgeval Tests for all registered packages should be run with this change label Sep 15, 2025
@nhz2
Copy link
Member Author

nhz2 commented Sep 15, 2025

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

@adienes adienes merged commit aa4cd70 into JuliaLang:master Sep 15, 2025
5 of 7 checks passed
@nhz2 nhz2 deleted the nz/complex-rational-div-1 branch September 15, 2025 23:36
xal-0 pushed a commit to xal-0/julia that referenced this pull request Sep 30, 2025
…#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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

complex Complex numbers minor change Marginal behavior change acceptable for a minor release rationals The Rational type and values thereof

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants