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

Add support for one-padding #252

Merged
merged 5 commits into from Mar 3, 2020
Merged

Add support for one-padding #252

merged 5 commits into from Mar 3, 2020

Conversation

Tombana
Copy link
Collaborator

@Tombana Tombana commented Feb 27, 2020

This adds support for setting the attribute pad_values = 1.

@Tombana Tombana added the feature label Feb 27, 2020
@Tombana Tombana requested a review from arashb Feb 27, 2020
@Tombana
Copy link
Collaborator Author

@Tombana Tombana commented Feb 27, 2020

As discussed with @lgeiger I'm fine with using SAME_ONE as attribute. (The distinction is not super important since users should never see that stuff, its only set by the converter).

Copy link
Member

@lgeiger lgeiger left a comment

Nice!
Yes, I think I would slightly prefer naming it padding="SAME_ONE", though padding="ONE" is also fine with me.

larq_compute_engine/tflite/tests/bconv2d_test.cc Outdated Show resolved Hide resolved
@Tombana Tombana added feature and removed feature labels Feb 28, 2020
larq_compute_engine/tflite/kernels/bconv2d.cc Outdated Show resolved Hide resolved
larq_compute_engine/tflite/kernels/bconv2d.cc Outdated Show resolved Hide resolved
larq_compute_engine/tflite/kernels/bconv2d.cc Outdated Show resolved Hide resolved
larq_compute_engine/tflite/tests/bconv2d_test.cc Outdated Show resolved Hide resolved
larq_compute_engine/tflite/tests/bconv2d_test.cc Outdated Show resolved Hide resolved
larq_compute_engine/tflite/tests/bconv2d_test.cc Outdated Show resolved Hide resolved
@arashb
Copy link
Contributor

@arashb arashb commented Feb 28, 2020

I am seeing in larq/larq#438 @lgeiger uses an additional argument to pass the padding values which is default to 0. I think for bconv we should use the same interface as well. basically in this way the padding type stays the same however, the padding default varlue for padding=SAME will be set according to our needs. We can check if the default value is zero or not, it its not then we do all the extra stuff that needs to be done.

@lgeiger
Copy link
Member

@lgeiger lgeiger commented Feb 28, 2020

I think for bconv we should use the same interface as well. basically in this way the padding type stays the same however, the padding default varlue for padding=SAME will be set according to our needs. We can check if the default value is zero or not, it its not then we do all the extra stuff that needs to be done.

I don't have a strong opinion on that, but I think I'd prefer to stick with padding="SAME_ONE" . For larq we opted to have an additional pad_values argument in order to easily support experimentation with different padding values, where as for compute engine we'll only ever support plus or minus one padding and we don't need to add an additional kernel argument.

@arashb
Copy link
Contributor

@arashb arashb commented Feb 28, 2020

I think for bconv we should use the same interface as well. basically in this way the padding type stays the same however, the padding default varlue for padding=SAME will be set according to our needs. We can check if the default value is zero or not, it its not then we do all the extra stuff that needs to be done.

I don't have a strong opinion on that, but I think I'd prefer to stick with padding="SAME_ONE" . For larq we opted to have an additional pad_values argument in order to easily support experimentation with different padding values, where as for compute engine we'll only ever support plus or minus one padding and we don't need to add an additional kernel argument.

my problem with padding="SAME_ONE" is that its not really a new padding type but its just changing the default value of padding elements in the SAME padding type. Defining it as a new padding-type is confusing and is causing hacky changes in the design of the software.

@lgeiger lgeiger added feature and removed feature labels Mar 2, 2020
@lgeiger lgeiger requested a review from arashb Mar 2, 2020
@Tombana
Copy link
Collaborator Author

@Tombana Tombana commented Mar 2, 2020

Rebased on master to be able to do the MLIR one-padding PR on top of this.

arashb
arashb approved these changes Mar 3, 2020
Copy link
Contributor

@arashb arashb left a comment

@lgeiger @Tombana After lots of discussion I finally understand why we are doing it this way. I think it would have been a lot better design if we have had the Larq/LCE in the following form:

  1. The Larq layer QuantConv accepts any float padding value
  2. During the training we pad the values then sign the tensor(going to +1,-1 in float space for the entire padded tensor) and compute the convolution
  3. with MLIR converter we detect which pad value is passed, then pass that value to BConv Op
  4. BConv op could decide based on the padding value how to do the correction (if needed at all bc correction is needed only if the padding in float space was 0 and every other padding value could have worked out of the box without any correction.)

Tombana and others added 2 commits Mar 3, 2020
* Implement one padding transformation

* Check spatial padding size before fusing

* Prefer ConstantOp over TF_ConstOp

* Correctly convert strided convolutions
@arashb arashb merged commit bafb7a8 into master Mar 3, 2020
6 checks passed
@arashb arashb deleted the one_padding branch Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants