fix spurious overflow for Float16(::Rational) #52395
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stevengj
commented
Dec 13, 2023
stevengj
commented
Dec 13, 2023
stevengj
commented
Dec 13, 2023
stevengj
commented
Dec 13, 2023
stevengj
force-pushed
the
rational_to_float16
branch
from
3c40acc to
4270c36
Compare
stevengj
force-pushed
the
rational_to_float16
branch
from
4270c36 to
c34a75d
Compare
nsajko
added a commit
to nsajko/julia
that referenced
this pull request
Jan 13, 2024
Implementation approach: 1. Convert the (numerator, denominator) pair to a (sign bit, integral significand, exponent) triplet using integer arithmetic. The integer type in question must be wide enough. 2. Convert the above triplet into an instance of the chosen FP type. There is special support for IEEE 754 floating-point and for `BigFloat`, otherwise a fallback using `ldexp` is used. As a bonus, constructing a `BigFloat` from a `Rational` should now be thread-safe when the rounding mode and precision are provided to the constructor, because there is no access to the global precision or rounding mode settings. Updates JuliaLang#45213 Updates JuliaLang#50940 Updates JuliaLang#52507 Fixes JuliaLang#52394 Closes JuliaLang#52395 Fixes JuliaLang#52859
nsajko
added a commit
to nsajko/julia
that referenced
this pull request
Jan 14, 2024
Implementation approach: 1. Convert the (numerator, denominator) pair to a (sign bit, integral significand, exponent) triplet using integer arithmetic. The integer type in question must be wide enough. 2. Convert the above triplet into an instance of the chosen FP type. There is special support for IEEE 754 floating-point and for `BigFloat`, otherwise a fallback using `ldexp` is used. As a bonus, constructing a `BigFloat` from a `Rational` should now be thread-safe when the rounding mode and precision are provided to the constructor, because there is no access to the global precision or rounding mode settings. Updates JuliaLang#45213 Updates JuliaLang#50940 Updates JuliaLang#52507 Fixes JuliaLang#52394 Closes JuliaLang#52395 Fixes JuliaLang#52859
nsajko
added a commit
to nsajko/julia
that referenced
this pull request
Jan 14, 2024
Implementation approach: 1. Convert the (numerator, denominator) pair to a (sign bit, integral significand, exponent) triplet using integer arithmetic. The integer type in question must be wide enough. 2. Convert the above triplet into an instance of the chosen FP type. There is special support for IEEE 754 floating-point and for `BigFloat`, otherwise a fallback using `ldexp` is used. As a bonus, constructing a `BigFloat` from a `Rational` should now be thread-safe when the rounding mode and precision are provided to the constructor, because there is no access to the global precision or rounding mode settings. Updates JuliaLang#45213 Updates JuliaLang#50940 Updates JuliaLang#52507 Fixes JuliaLang#52394 Closes JuliaLang#52395 Fixes JuliaLang#52859
nsajko
added a commit
to nsajko/julia
that referenced
this pull request
Jan 14, 2024
Implementation approach: 1. Convert the (numerator, denominator) pair to a (sign bit, integral significand, exponent) triplet using integer arithmetic. The integer type in question must be wide enough. 2. Convert the above triplet into an instance of the chosen FP type. There is special support for IEEE 754 floating-point and for `BigFloat`, otherwise a fallback using `ldexp` is used. As a bonus, constructing a `BigFloat` from a `Rational` should now be thread-safe when the rounding mode and precision are provided to the constructor, because there is no access to the global precision or rounding mode settings. Updates JuliaLang#45213 Updates JuliaLang#50940 Updates JuliaLang#52507 Fixes JuliaLang#52394 Closes JuliaLang#52395 Fixes JuliaLang#52859
nsajko
added a commit
to nsajko/julia
that referenced
this pull request
Jan 14, 2024
Implementation approach: 1. Convert the (numerator, denominator) pair to a (sign bit, integral significand, exponent) triplet using integer arithmetic. The integer type in question must be wide enough. 2. Convert the above triplet into an instance of the chosen FP type. There is special support for IEEE 754 floating-point and for `BigFloat`, otherwise a fallback using `ldexp` is used. As a bonus, constructing a `BigFloat` from a `Rational` should now be thread-safe when the rounding mode and precision are provided to the constructor, because there is no access to the global precision or rounding mode settings. Updates JuliaLang#45213 Updates JuliaLang#50940 Updates JuliaLang#52507 Fixes JuliaLang#52394 Closes JuliaLang#52395 Fixes JuliaLang#52859
nsajko
added a commit
to nsajko/julia
that referenced
this pull request
Jan 15, 2024
Implementation approach: 1. Convert the (numerator, denominator) pair to a (sign bit, integral significand, exponent) triplet using integer arithmetic. The integer type in question must be wide enough. 2. Convert the above triplet into an instance of the chosen FP type. There is special support for IEEE 754 floating-point and for `BigFloat`, otherwise a fallback using `ldexp` is used. As a bonus, constructing a `BigFloat` from a `Rational` should now be thread-safe when the rounding mode and precision are provided to the constructor, because there is no access to the global precision or rounding mode settings. Updates JuliaLang#45213 Updates JuliaLang#50940 Updates JuliaLang#52507 Fixes JuliaLang#52394 Closes JuliaLang#52395 Fixes JuliaLang#52859
nsajko
added a commit
to nsajko/julia
that referenced
this pull request
Jan 15, 2024
Constructing a floating-point number from a `Rational` should now be correctly rounded. Implementation approach: 1. Convert the (numerator, denominator) pair to a (sign bit, integral significand, exponent) triplet using integer arithmetic. The integer type in question must be wide enough. 2. Convert the above triplet into an instance of the chosen FP type. There is special support for IEEE 754 floating-point and for `BigFloat`, otherwise a fallback using `ldexp` is used. As a bonus, constructing a `BigFloat` from a `Rational` should now be thread-safe when the rounding mode and precision are provided to the constructor, because there is no access to the global precision or rounding mode settings. Updates JuliaLang#45213 Updates JuliaLang#50940 Updates JuliaLang#52507 Fixes JuliaLang#52394 Closes JuliaLang#52395 Fixes JuliaLang#52859
nsajko
added a commit
to nsajko/julia
that referenced
this pull request
Jan 15, 2024
Constructing a floating-point number from a `Rational` should now be correctly rounded. Implementation approach: 1. Convert the (numerator, denominator) pair to a (sign bit, integral significand, exponent) triplet using integer arithmetic. The integer type in question must be wide enough. 2. Convert the above triplet into an instance of the chosen FP type. There is special support for IEEE 754 floating-point and for `BigFloat`, otherwise a fallback using `ldexp` is used. As a bonus, constructing a `BigFloat` from a `Rational` should now be thread-safe when the rounding mode and precision are provided to the constructor, because there is no access to the global precision or rounding mode settings. Updates JuliaLang#45213 Updates JuliaLang#50940 Updates JuliaLang#52507 Fixes JuliaLang#52394 Closes JuliaLang#52395 Fixes JuliaLang#52859
fingolfin
approved these changes
Feb 7, 2024
vtjnash
approved these changes
Feb 7, 2024
| Float16(x::Rational{<:Union{Int128,UInt128}}) = | ||
| Float16(Float64(x)) # UInt128 overflows Float32, include Int128 for consistency | ||
| Float32(x::Rational{<:Union{Int128,UInt128}}) = | ||
| Float32(Float64(x)) # UInt128 overflows Float32, include Int128 for consistency |
There was a problem hiding this comment.
Wouldn't these suffer from double-rounding accuracy loss? @oscardssmith
There was a problem hiding this comment.
they would. Doing this properly is fairly hard though.
There was a problem hiding this comment.
Also worth noting that Float16(num)/Float16(den) also potentially has triple rounding.
There was a problem hiding this comment.
Question is: is this problem worse than the problem this PR fixes? I.e. should we merge and improve this later, or should this wait until this PR does it "right" (whatever that means?)
There was a problem hiding this comment.
we should merge and improve later.
There was a problem hiding this comment.
That was my thought as well, but want to confirm.
There was a problem hiding this comment.
Would doing it "properly" be something like:
a = Float64(x)
b = Float32(a)
# check if we double-rounded in the wrong direction
if x > a && b < a && nextfloat(b) < a
b = nextfloat(b)
elseif x < a && b > a && prevfloat(b) > a
b = prevfloat(b)
end
return bThere was a problem hiding this comment.
A example of a test case where double rounding gives a different result is:
julia> r = 12928845309018111//18014398509481984
12928845309018111//18014398509481984
julia> Float32(r) == Float32(Float64(r))
false@vtjnash's code doesn't fix this, however — its output matches Float32(Float64(r)) on this example.
(Example constructed by tweaking a number almost exactly halfway between two Float32 values.)
oscardssmith
removed
the
merge me
|
Was there a point to merging this? This PR was just a quick fix compared to the thorough fix of #49749. |
|
Primarily that it was a quick fix, compared to the thorough fix of #49749 :). To be slightly more serious, I think we should merge the more thorough fix, but this was an improvement that was ready to merge first. |
nsajko
added a commit
to nsajko/julia
that referenced
this pull request
May 12, 2024
Constructing a floating-point number from a `Rational` should now be correctly rounded. Implementation approach: 1. Convert the (numerator, denominator) pair to a (sign bit, integral significand, exponent) triplet using integer arithmetic. The integer type in question must be wide enough. 2. Convert the above triplet into an instance of the chosen FP type. There is special support for IEEE 754 floating-point and for `BigFloat`, otherwise a fallback using `ldexp` is used. As a bonus, constructing a `BigFloat` from a `Rational` should now be thread-safe when the rounding mode and precision are provided to the constructor, because there is no access to the global precision or rounding mode settings. Updates JuliaLang#45213 Updates JuliaLang#50940 Updates JuliaLang#52507 Fixes JuliaLang#52394 Closes JuliaLang#52395 Fixes JuliaLang#52859
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #52394.
Update: also fixes
Float32forUInt128, since currentlyFloat32((typemax(UInt128)-0x01) // typemax(UInt128))givesNan32.