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

Feat: Add QuantConv3d and QuantConv3dTranspose #805

Merged
merged 42 commits into from
Mar 7, 2024

Conversation

costigt-dev
Copy link
Collaborator

@costigt-dev costigt-dev commented Jan 23, 2024

Issue

Implements #795

Details

  • Extends QuantConv and QuantConvTranspose to have 3d variant.
  • Adds test for QuantConv3d
  • Updates output_channel_dim for QuantConv1d to no longer throw RuntimeError
  • Also removes convXd_same_zero_pad_stride as it is only invoked by an invalid combination of padding/stride parameters.
  • Adds tests for MergeBatchNorm and AvgPoolToQuantDepthwiseConv

@Giuseppe5 Giuseppe5 marked this pull request as ready for review January 23, 2024 17:09
@costigt-dev costigt-dev added next release PRs which should be merged for the next release and removed next release PRs which should be merged for the next release labels Feb 1, 2024
@costigt-dev costigt-dev requested a review from Giuseppe5 February 8, 2024 16:29
@nickfraser nickfraser removed the next release PRs which should be merged for the next release label Feb 12, 2024
group_size = self.out_channels // self.groups
overlapping_sums = max(round(self.kernel_size[0] / self.stride[0]), 1)
overlapping_sums *= max(round(self.kernel_size[1] / self.stride[1]), 1)
overlapping_sums *= max(round(self.kernel_size[2] / self.stride[2]), 1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually calculate the right thing? The way I read this function is that group_size * overlapping_sums should equal the number of terms in my dot product. If so, why is the stride important?

@i-colbert, @Giuseppe5, can you enlighten me here?

Copy link
Collaborator

@i-colbert i-colbert Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have only worked with the 2D case, but if it is a trivial extension of that then this is almost correct. Unlike the convolution, which traverses the input space in strides, the deconvolution traverses the output space in strides and creates overlapping sums that are a function of the kernel size and stride. This paper introduces an algorithm the traverses the output space of the deconvolution (see Algorithm 8) and you can see the stride skipping there. However, you can see that it should be a floor rounding rather than a halfway rounding. I would propose the following:

...
patch_size = (self.kernel_size[0] // self.stride[0]) * (self.kernel_size[1] // self.stride[1]) * (self.kernel_size[2] // self.stride[2])
max_uint_output = max_uint_input * max_kernel_val * patch_size * group_size
...

The stride shouldn't really outgrow the kernel_size, since it wouldn't make much sense unless there is a niche use case. Not sure if PyTorch throws an error in that case and also not sure if it'd be the job of the quant layer to protect against this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! <3

elif kwargs[
'model_type'] == 'QuantConvTranspose1d': # shape = (in_channels, out_channels, kernel_size)
quant_weight_per_channel_l1_norm = quant_weight.norm(p=1, dim=(0, 2))
elif kwargs[
'model_type'] == 'QuantConvTranspose2d': # shape = (in_channels, out_channels, kernel_size)
quant_weight_per_channel_l1_norm = quant_weight.norm(p=1, dim=(0, 2, 3))
elif kwargs[
'model_type'] == 'QuantConvTranspose3d': # shape = (in_channels, out_channels, kernel_size)
Copy link
Collaborator

@nickfraser nickfraser Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment: Should it be? # shape = (in_channels, out_channels, kernel_size, kernel_size)

Copy link
Collaborator

@nickfraser nickfraser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@costigt-dev costigt-dev requested review from capnramses and removed request for capnramses March 6, 2024 12:15
@Giuseppe5 Giuseppe5 self-requested a review March 6, 2024 22:29
@Giuseppe5 Giuseppe5 merged commit d981aa9 into Xilinx:dev Mar 7, 2024
347 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New features good first issue Good for newcomers nn Anything related to nn modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants