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

Porting TF fake_quant_with_min_max functions #20641

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

doncarlos999
Copy link

@doncarlos999 doncarlos999 commented Dec 13, 2024

Based on the discussion here: #20319 I started porting the fake_quant_with_min_max functions from tensorflow to keras3.
This PR contains those ported functions and the relevant tests from https://github.com/tensorflow/tensorflow/blob/master/tensorflow/compiler/tests/fake_quant_ops_test.py.

I didn't implement tf.quantization.fake_quant_with_min_max_vars as it looks the same as tf.quantization.fake_quant_with_min_max_args. But, I can add this one too if required.

For the CLA I am waiting on our CTO to add me to the Edge Impulse <-> Google CLA. But I figured that I can work on revisions to the PR in the meantime.

CC: @matpalm, @dansitu, @james77777778

* adds fake_quant_with_min_max functions from TF to keras3
Copy link

google-cla bot commented Dec 13, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 89.39394% with 7 lines in your changes missing coverage. Please review.

Project coverage is 81.96%. Comparing base (97c1c00) to head (34954b6).

Files with missing lines Patch % Lines
keras/src/quantizers/quantizers.py 92.98% 3 Missing and 1 partial ⚠️
keras/api/_tf_keras/keras/quantizers/__init__.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #20641   +/-   ##
=======================================
  Coverage   81.95%   81.96%           
=======================================
  Files         553      553           
  Lines       51458    51524   +66     
  Branches     7961     7964    +3     
=======================================
+ Hits        42174    42233   +59     
- Misses       7346     7352    +6     
- Partials     1938     1939    +1     
Flag Coverage Δ
keras 81.78% <87.87%> (+<0.01%) ⬆️
keras-jax 64.07% <87.87%> (+0.03%) ⬆️
keras-numpy 58.92% <75.75%> (+0.02%) ⬆️
keras-openvino 29.82% <19.69%> (-0.02%) ⬇️
keras-tensorflow 64.73% <87.87%> (+0.02%) ⬆️
keras-torch 64.10% <86.36%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

keras/src/quantizers/quantizers.py Outdated Show resolved Hide resolved
keras/src/quantizers/quantizers.py Outdated Show resolved Hide resolved
keras/src/quantizers/quantizers.py Outdated Show resolved Hide resolved
Copy link
Contributor

@james77777778 james77777778 left a comment

Choose a reason for hiding this comment

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

Hi @doncarlos999
I have left some comments.

Additionally, I think we still need fake_quant_with_min_max_vars, as it is used in TFMOT:
https://github.com/tensorflow/model-optimization/blob/master/tensorflow_model_optimization/python/core/quantization/keras/quant_ops.py#L340

keras/api/_tf_keras/keras/quantizers/__init__.py Outdated Show resolved Hide resolved
keras/api/_tf_keras/keras/quantizers/__init__.py Outdated Show resolved Hide resolved
keras/api/quantizers/__init__.py Outdated Show resolved Hide resolved
keras/api/quantizers/__init__.py Outdated Show resolved Hide resolved
keras/src/quantizers/__init__.py Outdated Show resolved Hide resolved
keras/src/quantizers/quantizers.py Outdated Show resolved Hide resolved
@@ -100,3 +100,759 @@ def test_quantize_and_dequantize(self):
)
# A loose assertion due to an expected quantization error
self.assertAllClose(qdq_values, values, atol=5e-1)

def _TestOp(
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use @parameterized.named_parameters and named_product to organize similar tests like this one:
https://github.com/keras-team/keras/blob/master/keras/src/ops/nn_test.py#L2355-L2365

Copy link
Author

Choose a reason for hiding this comment

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

I have reduced the number of tests by merging the logic into a single function but if you still think it would help to use named_parameters to organize the tests then I can add it.

keras/src/quantizers/quantizers_test.py Outdated Show resolved Hide resolved
keras/src/quantizers/quantizers_test.py Outdated Show resolved Hide resolved
keras/src/quantizers/quantizers_test.py Outdated Show resolved Hide resolved
@doncarlos999
Copy link
Author

@james77777778 thank you for the review. I'm working on revisions now.

Regarding the *_gradient functions, I added those as a way to test that the gradients that come from the main functions were being calculated correctly. Should we keep them just for testing purposes but not expose them in the public facing API? If not I will remove them.

@james77777778
Copy link
Contributor

Regarding the *_gradient functions, I added those as a way to test that the gradients that come from the main functions were being calculated correctly. Should we keep them just for testing purposes but not expose them in the public facing API? If not I will remove them.

We can test the gradients of fake_* functions using:

  • tensorflow: tf.GradientTape() + tape.gradient
  • torch: loss.backward() + variable.grad
  • jax: jax.grad

You can refer to this test for an example:
https://github.com/keras-team/keras/blob/master/keras/src/layers/core/dense_test.py#L584-L649

Using a different function, separate from the user-facing function, for testing purposes seems redundant and fragile to me. However, we should wait for calls from @fchollet

@doncarlos999
Copy link
Author

I agree that having two separate functions is fragile I simply kept the functions separate as that was how they were tested in the Tensorflow repo.
I will start adding tests based on your example in the meantime. Thank you.

@doncarlos999
Copy link
Author

@james77777778 I have addressed your previous comments.
I refactored the code into a single function to avoid duplication as the logic for the per_channel function on a 1 length array is the same as the other separate functions. If we need a separate function for handling parameters being passed in as float/int rather than single length arrays of floats/ints, then I can add the other function back (I have a previous commit that I can pull it back from).

@fchollet
Copy link
Collaborator

Thanks for the updates!

@james77777778 should we merge?

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

Successfully merging this pull request may close these issues.

5 participants