Skip to content

Conversation

LinusJungemann
Copy link
Member

Changed the implementation of the QuantAvgPool to conform to Xilinx/brevitas#1042.

The QuantAvgPool implementation was changed from a SumPool + Trunc to AvgPool + Trunc. As a result, an additional division is needed. As the accumulated values of the layer are divided by the the kernel size, this division cannot necessarily be implemented by a bit shift. After the division the bit shift of the truncation is still needed like before this fix.

@iksnagreb
Copy link

I am not sure I fully understand the pooling situation: We have average pooling and quantized average pooling - at least here in the backend - as AvgPoolFunction and QuantAvgPoolFunction. Isn't the whole point of having the QuantAvgPoolFunction to handle the special case of shifting instead of division? Then introducing an extra unrestricted division feels weird - couldn't we simply instantiate the AvgPoolFunction in case of arbitrary divisors? I don't see why it is necessary to keep the two divisions separate of each other, but maybe I am missing part of the picture?

@LinusJungemann
Copy link
Member Author

LinusJungemann commented Jul 24, 2025

This was the case before, when the QuantAvgPool in reality implemented a SumPool+Truc instead of an AvgPool+Trunc. This behavior was changed with Xilinx/brevitas#1042. Now a QuantAvgPool implements an AvgPool with an appended truncation node. In the case where input_bits=output_bits, AvgPool and QuantAvgPool implement the exact same behavior.

But yes, it could be worth it to discuss combining these two implementations into a single one and adding a seperate trunc node that implements just the shifting. (Or at least reuse the AvgPool implementation inside the QuantAvgPool implementation.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants