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

QKeras to QONNX converter redux #28

Merged
merged 50 commits into from
Feb 11, 2023
Merged

QKeras to QONNX converter redux #28

merged 50 commits into from
Feb 11, 2023

Conversation

jmduarte
Copy link
Member

@jmduarte jmduarte commented Nov 30, 2022

This is the PR for the QKeras to QONNX converter that has fixed some of the issues in the previous PR draft #7.
Quick Overview:

  • Original strategy: (1) “Strip” QKeras model of quantization attributes and store in the dictionary; (2) convert (as if plain Keras model) using tf2onnx; (3) Insert “Quant” nodes at appropriate locations based on a dictionary of quantization attributes
  • Works for fully-connected models; however stumbling blocks when applied to convolutional models (related to interference between how tf2onnx handles channels-last/channels-first transformations and inserted Quant nodes)

This branch addresses the problem with the convolution layers and provides new tests for checking the stability of the converter. Even with the current fixes, there are still some issues with the converter. Some have workarounds that are outlined below:

  • Quantized-Relu inserts redundant quant node when used as output activation of Dense/Conv2D layer.
    Workaround: To use Quantized-Relu as separate QActivation layers.

Screenshot 2022-12-22 at 5 05 25 PM

  • Quantized-Bits Node is not added when used in QActivation layers
    Workaround: Use Quantized bits only at the output of Dense/Conv2D layers

Screenshot 2022-12-22 at 5 09 02 PM

  • Threshold of 0.5 must be used when using Ternary Quantization (This is sometimes unstable even with t=0.5)

src/qonnx/__init__.py Outdated Show resolved Hide resolved
src/qonnx/__init__.py Outdated Show resolved Hide resolved
@maltanar
Copy link
Collaborator

Could you also update this with the latest main? I took the fail-fast changes from this PR plus a pinned a few more versions to keep things building smoothly.

@selwyn96
Copy link
Collaborator

selwyn96 commented Jan 17, 2023

Could you also update this with the latest main? I took the fail-fast changes from this PR plus a pinned a few more versions to keep things building smoothly.

tf2onnx does not support NumPy 1.24.1 (NVIDIA/TensorRT#2557), can we revert the dependency to NumPy 1.23.5 ?
Edit: Looks like this was fixed in tf2onnx 1.12.1. Will update accordingly

@jmduarte jmduarte requested review from maltanar and removed request for vloncar January 24, 2023 23:46
setup.cfg Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
tests/util/random_reseed.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@jmduarte
Copy link
Member Author

jmduarte commented Feb 1, 2023

Note, some additional diffs got introduced when upgrading pre-commit hooks (basically new black style), do we want to downgrade to previous black?

Copy link
Collaborator

@maltanar maltanar left a comment

Choose a reason for hiding this comment

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

LGTM!

@maltanar
Copy link
Collaborator

maltanar commented Feb 9, 2023

For making new users aware of any limitations, I would suggest to link the first post in this PR from README.md under the QKeras heading, unless you'd like to introduce a new section on the Sphinx docs? If this sounds good I can merge this PR and add that link to README.

@selwyn96
Copy link
Collaborator

selwyn96 commented Feb 9, 2023

Sounds good, I can add a new section in the documentation to include the limitations.

@maltanar maltanar merged commit aedc7c7 into main Feb 11, 2023
@maltanar
Copy link
Collaborator

Merged in its current state but @selwyn96 please feel free to open a new PR for new documentation.

@maltanar maltanar added this to the v0.2.0 milestone Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants