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

Assert all ptq-common bit widths are positive integers #931

Merged
merged 3 commits into from
Apr 10, 2024

Conversation

OscarSavolainenDR
Copy link
Contributor

In testing I found that ptq_common.py quantize_model bit widths can take negative and zero values. In the case of zero values, the model outputs NaNs, and in the case of negative values it is a bit more complicated (real values get outputted), but it is still undesired behavior.

This PR adds a constraint that all 'ptq_common/quantize_model' bit widths should be positive integers.

Copy link
Collaborator

@Giuseppe5 Giuseppe5 left a comment

Choose a reason for hiding this comment

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

I was about to suggest somehing like this to fix the issue.

Going a bit deeper, bit width requirements can be further specified, for example bias, weight, and activation bit width for integer quantization can only be greater than 3, since for Ternary and Binary quantization we need a different setup.
For floating point quantization, we can have also 1 bit for mantissa or exponent.

However these checks should probably live somewhere else in the code (e.g., the quantizer should verify that it's valid), so for now this assert should be sufficient.

@Giuseppe5
Copy link
Collaborator

Can you run the pre-commit hooks please?

@OscarSavolainenDR
Copy link
Contributor Author

Can you run the pre-commit hooks please?

Yep, have done so now!

@Giuseppe5 Giuseppe5 merged commit a8159f9 into Xilinx:dev Apr 10, 2024
22 checks passed
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