sparsemat / sprs

sparse linear algebra library for rust
Apache License 2.0
381 stars 45 forks source link

Modifying the default implementation for `MulAcc`. #323

Closed taketo1024 closed 1 year ago

taketo1024 commented 1 year ago

I'd like to use sprs with num-bigint, num-rational etc. However, sprs's matrix multiplication requires that the scalar type implements MulAcc, which BigInt and Rational types do not suffice.

We can make these types automatically implement MulAcc, if we change the code in mul_acc.rs from the current

impl<N, A, B> MulAcc<A, B> for N
where
    N: Copy,
    B: Copy,
    A: num_traits::MulAdd<B, N, Output = N> + Copy,
{
    fn mul_acc(&mut self, a: &A, b: &B) {
        *self = a.mul_add(*b, *self);
    }
}

to

impl<N, A, B> MulAcc<A, B> for N
where
    for<'x> &'x A: Mul<&'x B, Output = N>,
    N: AddAssign<N> 
{
    fn mul_acc(&mut self, a: &A, b: &B) {
        self.add_assign(a * b);
    }
}

Is this a bad idea? (regarding performance?) Thanks.

taketo1024 commented 1 year ago

I have pushed the modified version on my folked repo. https://github.com/taketo1024/sprs/tree/mul_acc_default. I will send a PR if this modification seems good.

mulimoen commented 1 year ago

That seems like a good idea. We can judge the performance impact once the PR is up

taketo1024 commented 1 year ago

Created PR. Closing this issue. Thanks!