Skip to content

Fix and test Float16 Rational comparison #58217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

LilithHafner
Copy link
Member

Found and fix recommended by Eric Demer.

Previously, we had this non-transitivity:

julia> Float16(6.0e-8) == big(1//16777216) == 1//16777216
true

julia> Float16(6.0e-8) == 1//16777216
false

From Eric,

This is presumably not the preferred way to inform you of such issues, but

like last time, the other ways I found have Terms that I am not willing to agree to
and
this time, I imagine it is in fact a bug, rather than just a foot-gun

.

Multiplication of Float16 s by the built-in BitInteger types hits the generic
::Number fallback, which just promotes the inputs and then multiplies the results.
These pairs of inputs promote to Float16, so for Int32,UInt32 and larger,
multiplying this way can give Inf16 even when [[the Float16 input is neither
NaN nor infinite] and [the exact product has absolute value at most 1]].

While the decision for the above specifically might be
"That's just what one gets for doing arithmetic with Float16."
, a current consequence of this is that
== between Float16 s and Rationals for the wide-enough built-in BitInteger types
gives wrong output, and as a result, isequal is not transitive:

https://paste.ee/p/LfxRlI7F has a paste of a REPL session demonstrating this.

Ideally, one would define arithmetic operations between
built-in BitFloats and built-in BitIntegers so that when there
are no nans and no infs, the output is == to the result of
convert both inputs to Rational{BigInt}, multiply those Rationals, and then round.

The much-easier-to-implement patch, on the other hand,
would be just defining ==(x::Float16,q::Rational) = ==(Float32(x),q)
, since the built-in BitInteger types are not large-enough
to cause the comparison issue with Float32:

If q is not a power of 2, then the count_ones part will be false,
else for the built-in BitInteger types, q.den is at most UInt128(2)^127.
This is strictly less than floatmax(Float32) .
When q.den is a power of 2 and less than floatmax(Float32),
q.den can be represented exactly and the only way for
multiplication by it to be inexact is exceeding the exponent range.
In such cases, the exact product is at least big"2"^128, so the Float product is Inf32,
and q is at most typemax(UInt128), since q.den is non-zero so q is finite.
typemax(UInt128) < big"2"^128 < Inf32 , so for Float32,
the x*q.den == q.num part still works even in these cases.

(Those who are curious about why I didn't agree to the Terms for the
other ways, can view my explanation at https://paste.ee/p/Y325uIJL .)

@LilithHafner LilithHafner added rationals The Rational type and values thereof bugfix This change fixes an existing bug float16 equality Issues relating to equality relations: ==, ===, isequal labels Apr 24, 2025
@oscardssmith
Copy link
Member

oscardssmith commented Apr 24, 2025

I think this isn't a Float16 issue. There is nothing different about the Float16 method than any other abstractFloat so I don't think this is correct.

Edit: this is method works OKish for other floating point types since Base is missing bitintegers big enough to represent inv(nextfloat(0f0))

Co-authored-by: Oscar Smith <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug equality Issues relating to equality relations: ==, ===, isequal float16 rationals The Rational type and values thereof
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants