nalgebra icon indicating copy to clipboard operation
nalgebra copied to clipboard

Buggy behavior with simd_clamp: it regards all lanes collectively rather than independently

Open tschundler opened this issue 3 years ago • 4 comments

dbg!(vector![0,2].simd_clamp(vector![-1,-1], vector![1,1]));

returns

vector![0, 2].simd_clamp(vector![- 1, - 1], vector![1, 1]) = [
    [
        0,
        2,
    ],
]

I'd expect [0,1]

Similarly:

[src/main.rs:45] vector![- 2, 2].simd_clamp(vector![- 1, - 1], vector![1, 1]) = [
    [
        -2,
        2,
    ],
]

I'd expect -1,1

It seems to only work if all values exceed the clamping vector:

vector![2, 2].simd_clamp(vector![- 1, - 1], vector![1, 1]) = [
    [
        1,
        1,
    ],
]

tschundler avatar Dec 10 '22 00:12 tschundler

I compared with simba's simd_clamp:

WideF32x4::from([1.0, 2.0, -1.0,
                0.0]).simd_clamp(WideF32x4::from([-1.0, -1.0, -1.0, -1.0]),
    WideF32x4::from([1.0, 1.0, 1.0, 1.0])) = WideF32x4(
    (1.0, 1.0, -1.0, 0.0),

which returns the expected lane-by-lane clamping

tschundler avatar Dec 10 '22 01:12 tschundler

Hi! nalgebra’s Vector itself isn’t supposed to be used as a SIMD entity. So the SimdPartialOrd implementation defaults to its blanket impl. which is equivalent to lexicographical ordering.

sebcrozet avatar Jan 14 '23 15:01 sebcrozet

Wouldn't it be better to implement partial ordering another way? (though ordering makes less sense for a vector than clamping, so why does it even do that?)

The existence of an API implies it would work in a reasonable manner. Having this weird behavior could become a Hyrum's Law problem in the future.

tschundler avatar Jan 18 '23 06:01 tschundler

Also running into this, the behaviour is particularly surprising when working with points. E.g. I'm trying to clamp a Point within image boundaries and getting:

use nalgebra::SimdPartialOrd;

type P = nalgebra::Point2::<i32>;

fn main() {
    let p = P::new(64, -63);
    let max_p = P::new(2600, 1731);
    let clamped = p.simd_clamp(P::new(0, 0), max_p);
    dbg!(clamped); // [64, -63] - point is still outside of the image
}

RReverser avatar Jan 02 '25 13:01 RReverser