Should minmax require Ord instead of PartialOrd

[phimuemue]

I think requiring only PartialOrd instead of Ord is not optimal for minmax:

    // Concern 1: different behaviour than min/max
    let vec = [std::f64::NAN, 1.];
    //vec.iter().min(); // does not compile (for good reasons imho)
    //vec.iter().max(); // does not compile (for good reasons imho)
    vec.iter().minmax(); // compiles, which is surprising
    
    // Concern 2: Order of elements is relevant
    println!("{:?}", vec![std::f64::NAN, 1.].iter().minmax()); // (NaN, 1.0)
    println!("{:?}", vec![1., std::f64::NAN].iter().minmax()); // (1.0, NaN)

The above code highlights my concerns:

• It is inconsistent with min/max.
• The order of the elements plays too important a role. This is particularly problematic for e.g. iterators visiting iterating elements in an arbitrary order (such as HashMap::keys).

The documentation clearly states that

The keys can be floats but no particular result is guaranteed if a key is NaN.

but the problem is present not only for floats, but for anything that does not fulfill Ord.

Wouldn't it be clearer to require Ord, thereby leaving the responsibility of dealing with PartialOrd to the callers? I know this is a breaking change

[Avi-D-coder]

I would advocate, just adding another note in the docs .

[bluss]

The decision to use PartialOrd is conscious, so that the caller can be judicious themselves. I think using Ord mostly has theoretical benefits, can you maybe explain what concrete problem you see with the current situation?

[phimuemue]

To be honest, I did not follow the discussion that lead to having PartialOrd, so I may be missing important arguments here.

However, I am a bit wary when it comes to problematic situations that could be prevented by the compiler. (I think silently failing float comparisons are problematic sometimes.) I remember fixing C++ bugs resulting from NaN comparisons, and thus I am glad that Rust informs me that I cannot simply use min on floats, leaving me the possibility to choose min_by and decide what to do for NaN myself.

My argument would be similar for minmax.

I think back then we did not yet have minmax_by which possibly made it a bit harder to use with PartialOrds. Now that we have it, I think it is a decent compromise. It raises awareness that "something nontrivial" is going on, but the caller can still obtain a minmax for PartialOrd via minmax_by.

So, I agree that it is a bit annoying that floats are not "simply comparable", but I think the advantages (i.e. being aware that floats (or other PartialOrds) are not simply comparable) outweight the slight un-ergonomics of minmax_by in this case.

[bluss]

Thanks for elaborating. It's not that itertools is ignorant of the problem, but it's my call to prioritize convenience for floats here — at the user's discretion. I lean a bit more toward trusting the programmer I guess, that itertools allows calling this on floats if you want to do so, and for me that's was the thinking behind the feature, to deliberately provide something that std doesn't -- like many other features in itertools. I'm interested in that numerics programming is convenient in Rust too.

[jswrenn]

to deliberately provide something that std doesn't

I've imported itertools for the express purpose of using minmax on floats so I wouldn't have to roll my own solution. I'm therefore also inclined to keep the PartialOrd bound.

I'm a little surprised that a min_max function with an Ord bound isn't provided by the standard library. It apparently had this method at one point? It seems to have been deprecated in 1.3.0 and removed entirely in 1.4.0.

I agree that the inconsistency with min and max is surprising. I'm prioritizing non-breaking proposed changes to itertools at the moment, but I'd be happy to revisit this in the future. A reasonable solution might be to deprecate minmax, and introduce extrema (with an Ord bound), partial_extrema (with a PartialOrd bound), and perhaps provide partial_min and partial_max, too.

[phimuemue]

@bluss @jswrenn Thanks for sharing your thoughts.

I agree that is not top-prio at the moment. Maybe we can wait and gather more opinions/data to reach a well-informed decision.

[TieWay59]

I meet the same problem lately, very confusing at the beginning.