Make min/max use isless even for Real

[timholy]

In #23155, we made the generic min/max use isless rather than <. This is motivated by the observation that the implementation

min(a, b) = cmp(a, b) ? a : b

implies that cmp satisfies the same properties as isless (either cmp(a, b) in which case we return a, otherwise either cmp(b, a) or they are equal, in which case returning b is correct).

However, this is not true for Real types, including AbstractFloat (due to NaN). Yet currently the default for Real uses <.

Part of the motivation here is that many packages seem to subtype Real when, theoretically, perhaps they shouldn't. Prominent examples include IntervalArithmetic.Interval and ForwardDiff.Dual. An example using intervals that would fail is the following: min(1..3, 2..4): the two intervals have non-empty intersection, so it's not/shouldn't be true that 1..3 < 2..4, in which case 2..4 would be returned (which is wrong by any measure).

To be honest, I don't expect this to be mergeable, as I bet it is breaking. But I think it's worth seeing if it passes tests (I haven't tried locally) and perhaps a PkgEval run.

[timholy]

@nanosoldier runtests()

[nanosoldier]

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

[simeonschaub]

Does this mean min(1., NaN) would be 1. whereas max(1., NaN) is still NaN? (ref #7866 which I ironically stumbled upon through https://2pi.dk/2016/05/ieee-min-max)

I wonder whether with this change we could get rid of the "bad zero" shenanigans in

julia/base/reduce.jl

Line 637 in 53a00f3

isbadzero(::typeof(max), x::AbstractFloat) = (x == zero(x)) & signbit(x)

though

[timholy]

@nanosoldier runtests(["Pyehtim", "FMI", "RemoteSensingToolbox", "ControlSystemsMTK", "GeometricIntegrators", "FourierSpaces", "OptimKit", "Controlz", "GeoIO", "MCPTrajectoryGameSolver", "Test", "TaijaPlotting", "QuantumCumulants", "FiniteVolumeMethod", "LoopVectorization"])

[timholy]

Does this mean min(1., NaN) would be 1. whereas max(1., NaN) is still NaN?

There are two methods higher in the priority list:

julia> methods(min, Tuple{T,T} where T<:Real)
# 3 methods for generic function "min" from Base:
 [1] min(x::T, y::T) where T<:Union{Float32, Float64}
     @ Base.Math math.jl:894
 [2] min(x::T, y::T) where T<:AbstractFloat
     @ Base.Math math.jl:878
 [3] min(x::T, y::T) where T<:Real
     @ promotion.jl:533

That said, you raise an interesting point. TBH I'm not really an expert on the nuances of < vs isless; naively I would have expected that either < or isless would be implemented, not both. Perhaps the right way to solve this would be to only have < but use a trait to encode whether the order is partial or total, and then dispatch on that for algorithms. (Any algorithm that requires a total order, but one is not available, could MethodError.) But of course that's not within the realm of possibility at the present time.

In other words, the way that I'd like for this to work is to implement min/max using isless, which implies a total order, and then any other types that don't have a total order need to come up with their own implementation of min/max---and they get notified that this is required via the MethodError about missing an implementation of isless. That would avoid the bizarre behavior you noted.

[nanosoldier]

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.