-
-
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
Implement im2col and update conv2d #134
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! I've left just some minor observations.
* horizontally scan matrix (vector of vectors) | ||
**/ | ||
template <typename T> | ||
void horizontal_scan(const vector<vector<T>>& src, vector<T>& dst) { |
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.
These operations might be easier to execute using a gsl::multi_span structure. For sure not for this PR, I don't know if they actually help. But they should prevent extra-copies
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 will check this out, maybe they will be helpful.
// CKKSVector utils | ||
m.def("im2col_encoding", | ||
[](shared_ptr<TenSEALContext> ctx, vector<vector<double>> &input, | ||
const size_t kernel_n_rows, const size_t kernel_n_cols, | ||
const size_t stride) { | ||
vector<vector<double>> view_as_window; | ||
vector<double> final_vector; | ||
size_t windows_nb = im2col(input, view_as_window, kernel_n_rows, | ||
kernel_n_cols, stride); | ||
vertical_scan(view_as_window, final_vector); | ||
CKKSVector ckks_vector = CKKSVector(ctx, final_vector); | ||
return make_pair(ckks_vector, windows_nb); | ||
}); | ||
|
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 not in this PR, but the encoding shouldn't be doing encryption, just encoding it into a vector, then encryption should be something separate
if (kernel.empty() || | ||
(any_of(kernel.begin(), kernel.end(), | ||
[](const vector<double>& i) { return i.empty(); }))) { | ||
throw invalid_argument("Kernel matrix can't be empty"); |
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.
What about something like this? to check whether the vector is a well organized matrix?
if (kernel.empty() || | |
(any_of(kernel.begin(), kernel.end(), | |
[](const vector<double>& i) { return i.empty(); }))) { | |
throw invalid_argument("Kernel matrix can't be empty"); | |
if (kernel.empty() || | |
(any_of(kernel.begin(), kernel.end(), | |
[](const vector<double>& i) { return i.size() == kernel[0].size(); }))) { | |
throw invalid_argument("Kernel matrix can't be empty"); |
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.
The test will be wrong in the case where kernel[0].size() == 0
.
in other hand horizontal_scan
always check if the rows have the same size.
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.
Nice work!
* implement matrix vertical and horizontal scan * Image Block to Columns implementation * update conv2d_im2col * im2col_encoding python binding * update conv2d_im2col python test * update Convolution notebook * linting * use const ref to prevent the copy
Description
im2col_encoding
, it will allow user apply a im2col on input matrix and compute the convolution layer using on TenSEAL, there's no need to pad the input and the kernel nor flatting them in Python.Affected Dependencies
None.
How has this been tested?
Checklist