Add left to right broadcast for Standard binary ops

[yanndupis]

Hey @jvmncs, is it something like that you had in mind?

@mortendahl, fyi, if we have discovered that ndarray for binary operation only broadcast the right tensors to the left and not the opposite (which is supported by numpy). This makes the MAPE metric in the linear regression fail because we try to divide a tensor of shape (1,) by a tensor of shape (n, 1). So trying to find a solution.

Thanks,

[rdragos]

Is MAPE used anywhere in production? If not I'd suggest we keep the things as they are and only use MAPE as the test when the entire pipeline is in Rust.
Since we're going to get rid of pymoose in anycase...

[jvmncs]

just about what I was thinking, small correction in the comment

[jvmncs]

this broadcast will already be checked & attempted when we try to do self.0/other.0, so it's not necessary to nest like this. try this:

fn div(self, other: StandardTensor<T>) -> Self::Output {
    match self.0.broadcast(other.0.dim()) {
        Some(self_broadcast) => StandardTensor::<T>(self_broadcast / other.0),
        None => StandardTensor::<T>(self.0 / other.0)  // this part will panic if other.0 cannot be broadcast into self.0
    }
}

[jvmncs]

I just realized that the documentation is incorrect, the doc says it will broadcast self to rhs, but in fact it's the other way around according to the comment I linked below (and verified by writing a simple test of the two). the code I have here should be correct though

[yanndupis]

Ah, that's right. Your code snippet makes sense. I can implement for the other binary ops. Thanks

[jvmncs]

@rdragos this is more about fixing a limitation of rust's ndarray library to match what we would expect from numpy/python. according to the library author they plan to implement this natively in ndarray but are waiting for a rust feature to move from nightly to stable rust-ndarray/ndarray#437 (comment)

[rdragos]

@jvmncs this makes sense! Good work 👍

[jvmncs]

good to go once it's passing CI

[jvmncs]

nit: I would consider renaming this to test_div_broadcasting (or even just test_broadcasting) to emphasize we are testing the broadcasting itself more than the div (since we rely on ndarray for op correctness)

will re-approve when rest of binary ops are implemented!

[yanndupis]

Hey @jvmncs, I added broadcasting to the other ops. I guess it should be ok for this PR (but happy to look into it tomorrow if we prefer), we could look into parametrized test. Here is a potential candidate: https://crates.io/crates/rstest.

[yanndupis]

Ok, I think it should be ready now @jvmncs. As per conversation we only add implicit broadcast (left into right) to Standard ops for now.

[jvmncs]

lgtm 👍