min_by requires Ord not PartialOrd

[neunenak]

It would be handy if Iterator::min_by required PartialOrd instead of Ord. This requirement makes it impossible to use with among other common types f32 and f64, which is annoying. I don't know if the best solution is to simply change the required trait to PartialOrd or to provide some other method min_by_partial_ord or something along those lines. The same reasoning should apply to Iterator::max_by

[thestinger]

It wouldn't return the correct results for floating point types. It requires Ord because the min and max functions require it. Like the other methods provided by the Iterator trait, these are convenience methods handling a common case. I don't think it's worth adding more methods when it's easy enough to do it with the min and max methods on Float via fold or for.

[nodakai]

Vec::sort_by() accepts a comparator function |&T, &T| -> Ordering without any mentions to mathematical rigor.

Maybe we can do the same for min_by() so that we can write

let ints: &[i32] = [-3, 0, 1, 5, -10];
assert_eq!(*ints.iter().min_by(|x, y| x.abs().cmp(&y.abs())).unwrap(), 0);

let floats: &[f64] = [-3., 0., 1., 5., -10.];
assert_eq!(*floats.iter().min_by(|x, y| x.abs().partial_cmp(&y.abs()).unwrap()).unwrap(), 0.);

[thestinger]

Sure, that's likely a saner definition.

cc #15311

[SimonSapin]

It’s not clear to me that min_by behaves correctly on types that have PartialOrd but not Ord, meaning whose comparison is not a total order. Floats do not have a total order because of NaN.

If however the values are known not to be NaN, the ordering is total. So a work around could be to have a NotNan wrapper type:

#[deriving(PartialOrd, PartialEq, Clone)]
struct NotNan<T> {
    value: T
}
impl<T> NotNan<T> where T: Float {
    fn new(value: T) -> Option<NotNan<T>> { 
        if value.is_nan() { 
            None
        } else { 
            Some(NotNan { value: value }) 
        }
    }
}
impl<T> Eq for NotNan<T> where T: Float {}
impl<T> Ord for NotNan<T> where T: Float {
    fn cmp(&self, other: &NotNan) -> Ordering {
        let &NotNan(a) = self;
        let &NotNan(b) = other;
        if a == b {
            Equal
        } else if a > b {
            Greater
        } else {
            Less
        }
    }
}

CC @aturon

[kmcallister]

Haskell's sortBy takes a comparison function. The function comparing allows you to sort by a property of each element, i.e.

sortBy (comparing len)

There's actually another way to do this: combine the basic compare method with the on function, as

sortBy (compare `on` len)

(compare on len) is an infix function application, equivalent to (on compare len).

[ucarion]

I don't see why we should add Haskell's on (its implementation is pretty impenetrable), but I think a good solution would be to have min_by accept a comparator function: |&T, &T| -> Ordering.

If we also add a comparing function, then finding the longest string in a list would be as simple as:

strings.iter().min_by(comparing(str::len))

Which is not much more complicated than what we have to do today. And PartialOrds would work with this version of min_by too.

[kmcallister]

I agree that on wouldn't be too useful in Rust; I was just spelling out the alternatives.

[steveklabnik]

I'm pulling a massive triage effort to get us ready for 1.0. As part of this, I'm moving stuff that's wishlist-like to the RFCs repo, as that's where major new things should get discussed/prioritized.

This issue has been moved to the RFCs repo: rust-lang/rfcs#675