-
-
Notifications
You must be signed in to change notification settings - Fork 156
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: complete dot operation #220
Conversation
// TODO: better implement broadcasting for mul first then would be | ||
// implemented similar to 1D-1D | ||
throw invalid_argument("1D-2D dot isn't implemented yet"); | ||
if (this_shape[0] != other_shape[0]) |
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.
Why isn't valid for
this_shape[0] == other_shape[1]
?
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 need the vector size to match the first dimension of the matrix to perform the dot product
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.
LGTM! just one question
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.
LGTM! just some observations
tenseal/cpp/tensors/ckkstensor.cpp
Outdated
throw invalid_argument("can't perform dot: dimension mismatch"); | ||
// TODO: remove boradcast when implemented in _mul | ||
auto other_copy = other->copy(); | ||
other_copy->_data.reshape_inplace( |
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.
This could be directly
auto other_copy = other->reshape( ...
tenseal/cpp/tensors/ckkstensor.cpp
Outdated
throw invalid_argument("2D-1D dot isn't implemented yet"); | ||
if (this_shape[1] != other_shape[0]) | ||
throw invalid_argument("can't perform dot: dimension mismatch"); | ||
// TODO: remove boradcast when implemented in _mul |
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.
small typo
if (this_shape[1] != other_shape[0]) | ||
throw invalid_argument("can't perform dot: dimension mismatch"); | ||
// TODO: remove boradcast when implemented in _mul | ||
auto other_copy = 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.
maybe
auto other_copy = other.reshape(...
directly?
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.
other here is a const and can't call reshape directly, just kept this, but the rest is updated
* add 1D-2D dot * fix * reshape before broadcast * fix: return * ignore protobuf changes * 2D-1D dot * separate dot ops again * lint * fix: pass the copy * typos and updates * lint * need to copy plain first Co-authored-by: Bogdan Cebere <bogdan.cebere@gmail.com>
Description
dot operations were missing 1D-2D and 2D-1D tensors because of the missing broadcasting operations. Now we have broadcasting and we can complete the dot method.
Checklist