-
Notifications
You must be signed in to change notification settings - Fork 47
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
fully generic MulAcc<A,B> - groundwork for Fully Generic Matrix/Vector element, Product and Accumulator Types - #284
Conversation
This looks great! Any clea rbenefit in using two type bounds in It would be awsome to see this working with your use-case, could you make an impl in |
thanks for reviewing the changes
My use case (explained below) definitely does need 2 types. EDIT: I've defaulted (EDIT2: r.e. "orphan rules.." we'll need to make impls' for the combinations for primitive types? just tried that and it didn't let me -perhaps conflicting with 'MulAcc', it might need newtypes all the way) bear in mind Dimension Checked maths aswell which some people like .. I see Rust now has const generics (yay!) making that possible, I've seen people write physics engines with that used all the way through. Also const generics let you write nice generic Fixed Point maths which could also go in here.
my use case is speculative , but let me elaborate: So to get as far as using "sprs" in my use case , I will need to refactor all steps to get the matrix mul working. the work I've done so far on it shows it's definitely possible, but it reuiqres a few reworks here and there (I saw another place where it looked like it did "to_owned()" to return an empty vector "if src was empty", like it was cloning the input, have to stop think to get the type right there.. etc) https://github.com/experiment9123/graphengine/blob/master/rustgraph/src/lib.rs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some smaller style fixes, otherwise looks good
If you rebase on master you'll get the CI fixes. If you could add a test where you use the |
ok i've added a test like this - |
I took the liberty of rebasing and fixing some minor nits. Thank you @experiment9123 for your work on this! I'm excited to see this implemented on |
thats great.. i'll grab the latest version and try and make my way back through the callgraph till we can use this in sparse matrix mul X sparse vec.. i suspect it'll be several PR's yet, but we might get it for a user-facing "sparse dot product" first.. |
Partial implementation of #283