Fixing the //(x::Number, y::Complex) one liner to accomodate silent overflows and division by zero/infinity #56478
Conversation
giordano
added
rationals
base/rational.jl
Outdated
| if isinf(real(y)) || isinf(imag(y)) | ||
| return 0//1 | ||
| end |
There was a problem hiding this comment.
If y is a complex integer isn't isinf here always false?
There was a problem hiding this comment.
In issue #56245 the errors were thrown for "incorrectly assumes y is finite and nonzero" so I added conditions for for infinite and 0.
There was a problem hiding this comment.
That only applies to the case where y is a Complex{<:Rational}
base/rational.jl
Outdated
| end | ||
| real_y = real(y) | ||
| imag_y = imag(y) | ||
| denom = Int32(abs(real_y))^2 + Int32(abs(imag_y))^2 |
There was a problem hiding this comment.
I added that to account for any overflow, but int16 would work perfectly fine as well!
There was a problem hiding this comment.
Hey, I’ve implemented scaling to prevent any overflow in calculations (removing any need for INT32. Could you take a look at my approach
applied scaling while calculating absolute values in order to prevent overflow at any step
|
doctest fails: |
base/rational.jl
Outdated
|
|
||
|
|
||
| function //(x::Number, y::Complex) | ||
| if((x//abs2(y))==0//1 || (x//abs2(y))==1//0) |
There was a problem hiding this comment.
Comparing with iszero, isinf, etc., should be cheaper than with ==.
fixed the overflow issue in my solution, implemented iszero and isinf wherever possible for lighter implementation
|
Hey, the overflow was due to converting complex floats to rationals, I have instead come up with a solution requiring no rational conversions :) |
base/rational.jl
Outdated
| if isinf(real(y)) || isinf(imag(y)) | ||
| return 0//1 | ||
| end |
There was a problem hiding this comment.
That only applies to the case where y is a Complex{<:Rational}
base/rational.jl
Outdated
| imag_y = imag(y) | ||
| m=max(abs(real_y),abs(imag_y)) | ||
| if(m==0) | ||
| return 1//0 |
There was a problem hiding this comment.
This should throw an error instead of returning 1//0
| @test (7 // complex(y)) == (7 // y) | ||
| end | ||
| @test Int8(8) // Int8(100)im == 0//1 - 2//25*im | ||
| end |
There was a problem hiding this comment.
Take a look at #53453 for more tests to add, you just need to replace / with //
|
okay, so I have added tests from #53453 (they work fine), removed the redundancy related to isinf(y) being true only for rational y, added a divide error for division by complex(0), and removed type instability from function //(x::Number, y::Complex{<:Integer}). |
added comments that were deleted unintentionally
fixed uint test which failed before
attempt to remove ambiguity which seems to be affecting build tests
fixes ambiguity by merging functions
type instability fixed
typ instability fix attempt
Fixes #53435 Fixes #56245. I have added conditions for division by zero and infinite handling in the case of complex numbers and overflow handling in the case of integer-based complex numbers. Further, I have added a few tests for the same :)