clamp: Turn ifelse into ternary

[barucden]

Fixes #54022

[KlausC]

The following test case is not covered by only replacing ifelse by ternary. The problem are the conversions between signed and unsigned integers of same size.

julia> clamp(typemin(Int), UInt)
ERROR: InexactError: check_top_bit(UInt64, -9223372036854775808)
Stacktrace:
 [1] throw_inexacterror(f::Symbol, ::Type{UInt64}, val::Int64)
   @ Core ./boot.jl:634
 [2] check_top_bit
   @ ./boot.jl:648 [inlined]
 [3] toUInt64
   @ ./boot.jl:759 [inlined]
 [4] UInt64
   @ ./boot.jl:789 [inlined]
 [5] convert
   @ ./number.jl:7 [inlined]
 [6] clamp
   @ ./REPL[3]:3 [inlined]
 [7] clamp(x::Int64, ::Type{UInt64})
   @ Base.Math ./math.jl:123

[barucden]

I see. That might be a bigger issue though:

julia> min(typemin(Int), typemin(UInt))
ERROR: InexactError: convert(UInt64, -9223372036854775808)
Stacktrace:
 [1] throw_inexacterror(func::Symbol, to::Type, val::Int64)
   @ Core ./boot.jl:772
 [2] check_sign_bit
   @ ./boot.jl:778 [inlined]
 [3] toUInt64
   @ ./boot.jl:889 [inlined]
 [4] UInt64
   @ ./boot.jl:919 [inlined]
 [5] convert
   @ ./number.jl:7 [inlined]
 [6] _promote
   @ ./promotion.jl:375 [inlined]
 [7] promote
   @ ./promotion.jl:400 [inlined]
 [8] min(x::Int64, y::UInt64)
   @ Base ./promotion.jl:494

How should we resolve the case when the result is not representable in the promoted type?

I understand that clamp(x::Int64, ::Type{UInt64}) = UInt64(max(x, 0)) would work, but is it the right solution?

[KlausC]

would work, but is it the right solution?

Maybe it is not needed, if you avoid unnecessary conversions in your approach. Sufficient set of test cases would be good!

[barucden]

Right, this case can be fixed just by reordering the comparisons:

function clamp(x::X, lo::L, hi::H) where {X,L,H}
    T = promote_type(X, L, H)
    v = (x < lo) ? convert(T, lo) : convert(T, x)
    return (v > hi) ? convert(T, hi) : v
end

Let me know if you can think of other edge-cases. I'll try to come up with some myself.

[barucden]

The potential issue is when x, lo, or hi cannot be represented in T = promote_type(X, L, H): one of them is negative and T happens to be unsigned. That is possible only if at least one of X, L, H is an unsigned type with at least as many bits as the maximum number of bits of the other, signed types, e.g., promote_type(UInt64, Int32, Int64) (which is UInt64) or promote_type(UInt128, Int64, Int64) (which is UInt128), but not promote_type(UInt32, Int64, Int64) (which is Int64).

With the clamp version from my previous post, there are three possible cases that result in an error:

condition	failed convert	example call
x < lo	convert(T, lo)	clamp(Int64(-2), Int64(-1), UInt64(1))
lo <= x <= hi	convert(T, x)	clamp(Int64(-2), Int64(-3), UInt64(1))
x > hi	convert(T, hi)	clamp(UInt64(1), Int64(-4), Int64(-3))

Note that if we permit lo < hi, there is another possible error if 0 > x > hi and lo is unsigned, e.g., clamp(Int64(-1), UInt64(1), Int64(-3)).

All of the examples listed here also fail in Julia v1.10.2.

[KlausC]

The docu of clamp says:

  Return x if lo <= x <= hi. If x > hi, return hi. If x < lo, return lo. Arguments are promoted to
  a common type.

Maybe it is sufficient to just do

function clamp(x::X, lo::L, hi::H) where {X,L,H}
    T = promote_type(X, L, H)
    x > hi ? convert(T, hi) : x < lo ? convert(T, lo) : convert(T, x)
end

The promotion to a common type have to be done by explicit conversions.

For the other signature that would imply a conversion hi or lo to the type of x, which can fail. Therefore

clamp(x, ::Type{T}) where T<:Integer = x > typemax(T) ? typemax(T) : x < typemin(T) ? typemin(T) : T(x)

should do.

As a proof of concept and test cases I have:

julia> function clamp1(x::X, lo::L, hi::H) where {X,L,H}
           T = promote_type(X, L, H)
           x > hi ? convert(T, hi) : x < lo ? convert(T, lo) : convert(T, x)
       end
clamp1 (generic function with 2 methods)

julia> clamp1(x, ::Type{T}) where T<:Integer = x > typemax(T) ? typemax(T) : x < typemin(T) ? typemin(T) : T(x)
clamp1 (generic function with 2 methods)

julia> clamp1(Int16(-1), UInt16)
0x0000

julia> clamp1(typemax(UInt16), Int16)
32767

julia> clamp1(-1, 2, UInt(0))
0x0000000000000002

All 3 test cases throw Exception for the current implementations of clamp.

[barucden]

Is the method clamp(x, ::Type{T}) where T<:Integer really necessary?

The promotion to a common type have to be done by explicit conversions.

So you don't see the following methods failing as an issue?

clamp(Int64(-1), Int64(-2), UInt64(1))
clamp(Int64(-2), Int64(-1), UInt64(1))
clamp(UInt64(1), Int64(-4), Int64(-3))

[KlausC]

I think, the following cases should work. Please can you add the test cases:

julia> clamp(Int16(-1), UInt16)
0x0000

julia> clamp(-1, 2, UInt(0))
0x0000000000000002

julia> clamp(typemax(UInt16), Int16)
32767

julia> clamp(2.0^63, Int)
ERROR: MethodError: no method matching rem(::Float64, ::Type{Int64})

The last one indicates IMO a bug in clamp(x,::Type{<:Integer} but is avoided by the implementation I proposed.

[KlausC]

So you don't see the following methods failing as an issue?

Yes. I think this is a consequence of the specs of clamp: "Arguments are promoted to a common type."
That makes sense for example if x is a float and lo, hi are integers.
In your cases it is a bit unfortunate, but I think acceptable.

[barucden]

Please can you add the test cases:

Yes, I will!

julia> clamp(2.0^63, Int)
ERROR: MethodError: no method matching rem(::Float64, ::Type{Int64})

The last one indicates IMO a bug in clamp(x,::Type{<:Integer} but is avoided by the implementation I proposed.

That's due to % T in

julia/base/math.jl

Line 123 in 1ae41a2

clamp(x, ::Type{T}) where {T<:Integer} = clamp(x, typemin(T), typemax(T)) % T

What if we simply changed that definition to

clamp(x, ::Type{T}) where {T<:Integer} = convert(T, clamp(x, typemin(T), typemax(T)))

[KlausC]

What if we simply changed that definition to

That also looks functionally good.
I hesitate to prefer that for the case clamp(big(2)^200, Int).
Your proposal would first convert typemax(Int) to BigInt, then the outer convert undo that converting BigInt to Int.
That might be inefficient. The solution I proposed avoids that to-and-fro conversions.

[barucden]

Right, I see. I wanted to avoid duplicating the comparison logic but your argument is strong.

[KlausC]

LGTM - should be merged