sparsemat / sprs

sparse linear algebra library for rust
Apache License 2.0
416 stars 47 forks source link

suggestion - add num::Zero or Default constraint to MulAcc #285

Open experiment9123 opened 3 years ago

experiment9123 commented 3 years ago

imagine trait MulAcc : num_traits::Zero{..} - the types satisfying this (for dot product output and matrix multiply output) usually want to be zero-able to initialize them. Often we see bounds like this N: 'a + Clone + crate::MulAcc + num_traits::Zero, (infact would it be reasonable to demand that an accumulator type is Cloneable?)

many of the helper functions throughout need to qualify this extra Zero constraint because they do this intialization.

As an alternative for further generality you might want to consider "Default" rather than "Zero"- this would further open up options in fitting other calculations to the pattern of matrix-multiply. Default kind of means "empty", e.g. Vec::default()=vec![]. an "empty" accumulator is zero. it's also a core trait so more likely a user has already got it implemented.

i've verified that the stdlib provides i32::default()==0 f32::default()==0, f64::default()==0 .. put numeric fields in a struct with #[derive(Default)] and you get them cleared. https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=37acf55a89cde8c5b066736c2f3d5142

I haven't put this into my PR for MulAcc<A,B> as I dont want to flood too many changes at once (especially the idea of default instead of zero..)

mulimoen commented 3 years ago

I think we use a combination of Default and Zero mixed around the codebase, but they serve two different purposes. Default should be exposed when we need to build a structure, and then replace the contents, Zero when we want to start accumulation from a zero. They are not required to be equal, therefore we distinguish these cases.

experiment9123 commented 3 years ago

ok fair enough. would it be overkill to do trait MulAcc : Zero+Default{} ? perhaps various parts of the code only require one or the other and you prefer to keep it seperable (eg incase of a future split ).

mulimoen commented 3 years ago

Either we have to require Mul for MulAcc, or we can require Zero. You can safely add Zero as a trait bound for MulAcc.

I think places we have Default, we could instead use MaybeUninit and a bit of unsafe. We should see if this makes the API more convenient