spurious overflow in Rational-to-Float16 conversion

[stevengj]

It would be nice if this gave a correctly rounded result rather than overflowing:

julia> Float16(10^9 // (10^9 + 1)) # spurious overflow
NaN16

julia> Float16(Float64(10^9 // (10^9 + 1))) # correctly rounded
Float16(1.0)

It's not specific to Float16 arithmetic, in principle, but that's where it shows up most easily since Float32 and Float64 cannot be overflowed by Int.

(I ran into this when implementing a recursive pairwise mean in #52365 (comment))

[stevengj]

A simple workaround would be to define something like:

Float16(x::Rational{<:Union{Int16,Int32,Int64,Int128,UInt16,UInt32,UInt64,UInt128}}) = Float16(Float32(x))

[nsajko]

#49749 is supposed to fix this as far as I remember, but I still haven't gotten around to finishing it.

[andrewjradcliffe]

It's not specific to Float16 arithmetic, in principle, but that's where it shows up most easily since Float32 and Float64 cannot be overflowed by Int.

Indeed, one has to go out of their way to cause it with Float32, but the same problem exists for any Rational{UInt128} where denominator > numerator and numerator >= 2^(emax + 1) - 2^(emax - precision), i.e. numerator >= the largest finite floating point number (2^(emax + 1) - 2^(emax + 1 - precision)) plus half a unit in the last place (which in the last binade is 2^(emax - precision)).

Float32 has a maximum exponent of 127 and precision of 24, thus, the condition is numerator >= 2^128 - 2^103.

# unsigned wraparound makes this succinct. Or use `BigInt` and convert to `UInt128`.
Float32(-(UInt128(1) << 103) // (-(UInt128(1) << 103) + 1))

# Naturally, this only applies to numerators that satisfy the above condition
# after reduction to smallest terms.
Float32(-(UInt128(1) << 103) // (-(UInt128(1) << 103) + 2))

One should leave out Int16 from the blanket definition for Float16 conversion. Float16's maximum exponent of 15 and precision of 11 imply numerator >= 2^16 - 2^4, which is greater than the magnitude of any Int16.

[vtjnash]

Will this help with #36423? Would you be willing to take a look at #51944 as well and see if that is okay to merge? Seems like we have had a few Rational-related PRs piling up without a confident review