-
-
Notifications
You must be signed in to change notification settings - Fork 157
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
CKKSTensor Dot operation #196
Conversation
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.
Great work!
just a few observations
shared_ptr<CKKSTensor> matmul(const shared_ptr<CKKSTensor> other) { | ||
return this->copy()->matmul_inplace(other); | ||
} | ||
shared_ptr<CKKSTensor> matmul_inplace(const shared_ptr<CKKSTensor> other); |
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.
There is something similar in the encrypted_vector interface. Maybe move them to the encrypted_tensor interface?
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.
True! This will require removing the n_jobs parameter from matmul, which I can't remember why I chose to put it there... I will open another PR for the API change after this one.
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.
Awesome work! LGTM
} | ||
return true; | ||
}; | ||
|
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.
we should somehow reuse the workers code, it is a lot of duplication here.
but for another PR.
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.
Yeah, I literally did a copy/paste here. I will open two refactor issues that I've found useful to work on
* first skeleton for dot operation * matmul * lint * tests * fix: create acc ct based on result param_id * lint * rename test func * fix indexing * redef API for dot dot_product* is now just dot* * lint * plain dot * comments * bind plain dot * parallel matmul * fix: to_sum should be scoped in the worker_func only
Description
This PR adds the dot operation between a CKKSTensor and another, or a PlainTensor. The shapes supported now are 1D-1D (inner product) and 2D-2D (matrix multiplication).
TODO
Fixes #176
How has this been tested?
Checklist