Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
experiment9123 opened this issue Apr 24, 2021 · 3 comments
Open

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

experiment9123 opened this issue Apr 24, 2021 · 3 comments

Comments

@experiment9123
Copy link

experiment9123 commented Apr 24, 2021

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 experiment9123 changed the title suggestion - add num::Zero constraint to MulAcc suggestion - add num::Zero or Default constraint to MulAcc Apr 24, 2021
@mulimoen
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants