isapprox does not handle typemin(::FixedPoint{<:Signed}) correctly

[kimikage]

For example,

julia> isapprox(-0.25Q0f7, -0.5Q0f7, rtol=0.5, atol=0) # this is OK
true

julia> isapprox(-0.5Q0f7, -1Q0f7, rtol=0.5, atol=0)
false

julia> isapprox(-0.5, -1.0, rtol=0.5, atol=0)
true

The direct cause is an overflow within abs.

FixedPointNumbers.jl/src/FixedPointNumbers.jl

Lines 93 to 97 in 3e41a6a

	function isapprox(x::T, y::T; rtol=0, atol=max(eps(x), eps(y))) where {T <: FixedPoint}
	maxdiff = T(atol+rtol*max(abs(x), abs(y)))
	rx, ry, rd = reinterpret(x), reinterpret(y), reinterpret(maxdiff)
	abs(signed(widen1(rx))-signed(widen1(ry))) <= rd
	end

julia> abs(-1Q0f7)
-1.0Q0f7

julia> abs(Int8(-128))
-128

I don't think this is a critical issue, but I plan to add checked_abs / saturating_abs after PR #190. Also, in the future, the default arithmetic may be changed to the checked arithmetic.

[kimikage]

This seems to be omitted by the compiler, but atol=max(eps(x), eps(y)) is verbose.
Also, since the difference between the n-bit values is in n bits, I think widen1 is excessive.

The current constructor is basically based on RoundNearest, but in this case it should be based on RoundDown or RoundToZero.

FixedPointNumbers.jl/src/FixedPointNumbers.jl

Line 94 in 3e41a6a

maxdiff = T(atol+rtol*max(abs(x), abs(y)))

I plan to partially support the constructors with RoundingMode option, but such constructors are unsuitable in this case because they check the input range.

[kimikage]

BTW, the tolerance in Base implementation is max(atol, rtol*max(norm(x), norm(y))). Is the difference (i.e. max/+) intentional?

cf.
PR #59 (Oct. 8, 2016)
JuliaLang/julia#22742 (Jul. 11, 2017)

[timholy]

Also, in the future, the default arithmetic may be changed to the checked arithmetic.

What's your plan for eliminating the performance hit?

Is the difference (i.e. max/+) intentional?

Probably not. As you deduced, it likely didn't follow changes in Julia itself.

[kimikage]

What's your plan for eliminating the performance hit?

It is to give up.:sweat_smile: In any case, we haven't reached the stage of deciding that yet. PR #190 is just a small step.

For this issue, since rtol is usually in Float64 if it is not 0, I will change abs(x) to abs(float(x)).

[timholy]

Just FYI, JuliaImages couldn't tolerate a big performance hit. We'd have to fork the package and go our own way.

[kimikage]

Just FYI, JuliaImages couldn't tolerate a big performance hit.

When was that? We still have things to do.

Arithmetic performance in JuliaImages should strongly depend on ColorTypes, Colors, ColorVectorSpace as well as FixedPointNumbers. For example, ColorBlendModes has a "non-slow" multiplication for N0f8/N0f16 components in advance (cf. #174).

[timholy]

When was that?

I meant, "couldn't" in the sense of "would not be able to," not something that happened in the past. Meaning, if arithmetic suddenly got 5x slower that wouldn't be something we'd be happy about.

I suppose it would be worth checking to see how often it actually comes up. In many applications we probably multiply by a float first, at which point it leaves the domain of FPN.