nalgebra: Wrong result of nalgebra::base::Matrix::min() and ::max()

The methods nalgebra::base::Matrix::min() and nalgebra::base::Matrix::max() are documented as

Returns the component with the smallest value.

and

Returns the component with the largest value.

However, A.min() actually returns N::zero().min(A[..]) (if this was a valid expression) and A.max() returns N::zero().max(A[..]). For example, this corresponds to

na::Vector3::new(-1.0, -2.0, -3.0).max() returning 0.0
na::Vector3::new(1.0, 2.0, 3.0).min() returning 0.0

This is due to the fact that both rely on xcmp which always initializes the comparison value with N::zero() instead of e.g. the first value of the matrix. This is either a bug in the code or in the documentation.

Currently I have to use A[A.imin()] instead of A.min().

About this issue

Original URL
State: open
Created 5 years ago
Comments: 15 (8 by maintainers)

Most upvoted comments

What about try_min() returning an Option, and min panicking if the matrix has no elements? That way, you can choose a sensible value when the matrix is empty by e.g. .try_min().unwrap_or(0.0).

As for the NaN question: PartialOrd is designed such that it gives an Option<Ordering>. Thus, one could consider something like

for e in iter {
    if let Some(ordering) = e.cmp(min) {
        if ordering == Ordering::Less {
            min = e;
        }
    } else if e.is_nan() {
         // This way we don't check `is_nan` unless the two elements are "uncomparable",
         // which for e.g. floating point numbers I believe only happens with NaNs?
         return e;
    }
}

As mentioned in the above comment in the code, this way you would only check explicitly for NaN when you have reason to suspect that the element might be NaN. How this actually ends up optimizing in the end is anybody’s guess though I suppose.

Andlon on Aug 9, 2019