fix float vs rational comparisons

[simonbyrne]

This (should) fix FloatingPoint vs Rational comparisons (mentioned in #2960 and #8411), and should be easily extendable to other binary Real types such as FixedPointNumbers.jl.

It does change the dispatch of comparison operators considerably, so there is a chance that this could break something, or cause a performance regression.

[simonbyrne]

Okay, I've fixed the issues and rebased. Should be working now.

[JeffBezanson]

It could be a serious problem that defining < and promote_rule for new types is no longer sufficient to get these kinds of cases to work. Maybe the new (Real,Real) fallbacks should use Union(Rational,FloatingPoint) instead? Or maybe we need a new Fractional type between Real and Rational (and FloatingPoint)?

[simonbyrne]

The reason for this are the calls to sign, which require <(::NewType,::Integer). I might replace these with explicit x > zero(x) checks, or define ispos/isneg methods (the advantage of the latter is that for BigFloat and BigInt it is possible to do the check without creating a zero(x)). Then the Integer comparisons would not be required for new types.

The problem with promote is that in a lot of cases, there isn't going to be a single type which encompasses two distinct Real types (e.g. BigFloat can't exactly represent Rationals). The decompose-based approach seems like the most general construction, and would cover almost all cases (including <(::NewType,::Integer)): the only exceptions I can think of are irrationals (like MathConst or @jiahao's suggested Surd type) and decimal floating point (which would require something like this).

[JeffBezanson]

x > 0 should work for every type; zero(x) should not be necessary there. Usually it will use promotion, but for example GMP has functions for comparing against small integers that we should use.

[simonbyrne]

The problem with relying on x > 0 is that <(::Integer,::NewType) then also needs to be specifically defined for each NewType.

[JeffBezanson]

Ah, I see now.

[nolta]

• Shouldn't prevfloat/nextfloat take a type argument? Then you could handle BigFloats with the same mechanism.
• I'm not totally keen on the names prevfloat/nextfloat, but can't think of obvious alternatives. My slightly nutty suggestion is floor(T,x)/ceil(T,x).
• Should decompose be exported?

[simonbyrne]

Shouldn't prevfloat/nextfloat take a type argument? Then you could handle BigFloats with the same mechanism.

Yes, I'm rewriting this to use stagedfunctions, which should make this clearer.

I'm not totally keen on the names prevfloat/nextfloat, but can't think of obvious alternatives. My slightly nutty suggestion is floor(T,x)/ceil(T,x).

I concede it's not fantastic, but floor and ceil seem even more of a stretch. Perhaps a better option would be to add a rounding mode option to convert, i.e.

convert{T<:FloatingPoint,s}(::Type{T}, ::MathConst{s}, r::RoundingMode)

but the current structure wouldn't be very efficient, as rounding modes are values, not types (so we can't dispatch on them). Perhaps that's worth changing?

Should decompose be exported?

Hopefully nobody should ever have to call this directly. If they do, perhaps we should consider making it an immutable type?

[simonbyrne]

Okay, it should all be working now, bar Rational vs. MathConst comparisons, which is going to be a bit more complicated. This also fixes a problem with hash for negative floats (they were giving InexactErrors): if this is going to take a while, I can split it out into a separate PR.

[nolta]

Int64?

[simonbyrne]

Good catch, thanks.

[nolta]

Instead of convert, how about round(T, ::MathConst, ::RoundingMode)? Or maybe roundup/rounddown?

The reasons i don't like prevfloat/nextfloat:

• prevfloat(x::T) returns T, unless T is MathConst.
• prevfloat(Rational, x) returns rational not float.

[nolta]

This may not be a problem, but on this branch

julia> using FixedPointNumbers
ERROR: `decompose` has no method matching decompose(::UfixedBase{Uint8,8})
 in cmp at hashing2.jl:240
 in < at hashing2.jl:286
 in > at operators.jl:33
...
while loading /scratch/nolta/julia-pkg/FixedPointNumbers/src/ufixed.jl, in expression starting on line 148
while loading /scratch/nolta/julia-pkg/FixedPointNumbers/src/FixedPointNumbers.jl, in expression starting on line 42

[simonbyrne]

I think overloading round suffers the same problems as floor/ceil: this behaviour differs so much from the current functions that they don't really make sense. prevfloat(Rational, x) isn't defined at the moment (and doesn't really make sense anyway, unless you were to bound the denominator).

I'm working on a PR for FixedPointNumbers.

[simonbyrne]

I'm in the process of cleaning this up and rebasing, but something has occurred to me: this could be a bit simpler if we were to have hash(0.0) == hash(-0.0). Let me explain...

Fewer internal comparisons would be required if the "denominator" from decompose was strictly non-negative (since the crossproduct wouldn't change the sign). Also, for a few things like Float64 vs Int128 comparisons, we could avoid promotion to BigInts by defining the denominator of a Float64 to be a Bool (since we can define widemul(::Int128,::Bool) to return an Int128 value).

As far as I can tell, the only reason we need negative denominators is to distinguish between signed zeros. Is this useful? Also, one additional advantage of hashing them to be equal is that hash could then be used for both isequal and == comparisons.

@StefanKarpinski thoughts?

[JeffBezanson]

hash(0.0) == hash(-0.0) sounds like a good idea. Consistency with both == and isequal is probably more valuable than avoiding that specific collision.

[StefanKarpinski]

Yes, I suppose that would be ok. I have to think about it a bit and recall what my thinking was at the time, but I do think that allowing a negative denominator was the only reason – 0/-1 is the "decomposition" of -0.0 while 0/1 is the decomposition of 0.0.

[simonbyrne]

Closed in favour of #9198