sprs icon indicating copy to clipboard operation
sprs copied to clipboard

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

Open experiment9123 opened this issue 3 years ago • 3 comments

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..)

experiment9123 avatar Apr 24 '21 17:04 experiment9123

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.

mulimoen avatar Apr 25 '21 10:04 mulimoen

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 ).

experiment9123 avatar Apr 25 '21 11:04 experiment9123

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

mulimoen avatar Apr 25 '21 17:04 mulimoen