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

micro: port op Reduce Prod from lite v2 #2527

Closed
Evanok opened this issue Mar 28, 2024 · 6 comments
Closed

micro: port op Reduce Prod from lite v2 #2527

Evanok opened this issue Mar 28, 2024 · 6 comments
Assignees
Labels

Comments

@Evanok
Copy link

Evanok commented Mar 28, 2024

Hello,

I would like to port the Reduce Prod operation from lite to micro. It should be quite straightforward even if I am not very familiar with tensor operation because other similar reduce operation are already ported.
There is already an issue about this porting but it seems to be abandon.

I already started the job but I am a little bit lost about the Quantized Reduce Prod function.
First I did an implementation inspired from QuantizedMeanOrSum and ReduceSumImpl function.
Then I realized there is already a function named QuantizedReduceProd in the reference reduce.h header: here

Should I used this existing function? Should I remove it and implement a new one based on the SumOrMean implementation?

Thanks,
Arthur.

@Evanok
Copy link
Author

Evanok commented Apr 1, 2024

First draw of the PR can be found here: PR #2532.
TODO:

  • Fix Quantized version which is not working properly.
  • Add missing test to match the reduce SUM test.

@Evanok
Copy link
Author

Evanok commented Apr 2, 2024

I still have issue with the Quantized reduce prod test.
I write a very simple test based on the SumInt82DKeepDims in reduce_test.cc

kInputData2D is {1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0};

TF_LITE_MICRO_TEST(ProdInt82DKeepDims) {
  int8_t expected_output_data_quant[tflite::testing::kOutputElements2D];
  int8_t output_data_quant[tflite::testing::kOutputElements2D];
  int8_t input_data_quant[tflite::testing::kInputElements2D];

  float input_scale = 0.5f;
  int input_zero_point = 0;
  float output_scale = 0.5f;
  int output_zero_point = 0;

  TfLiteReducerParams params = {
      true  // keep_dims
  };

    tflite::testing::TestReduceOpQuantized<int8_t>(
      tflite::testing::kInputShape2D, tflite::testing::kInputData2D,
      input_data_quant, input_scale, input_zero_point,
      tflite::testing::kAxisShape2D, tflite::testing::kAxisData2D,
      tflite::testing::kOutputShape2D, tflite::testing::kGoldenDataProd2D,
      output_data_quant, expected_output_data_quant, output_scale,
      output_zero_point, tflite::Register_PROD(), &params, 1.0);
}

Then I have an error during the test:
expected_output_data[i] (48.000000) near output_data[i] (127.000000) failed at tensorflow/lite/micro/kernels/reduce_test.cc:94

For me the output should be 127, 127. The expected output seems to be 48, 127.

From my understanding if I am computing the reduce prod of the input, I am quantified the input first.
With a scaling of 0.5. the Quantified input is {2.0, 4.0, 6.0, 8.0, 10.0, 12.0, 14.0, 16.0}
So the result is

2 * 4 * 6 * 8 -> 384
10 * 12 * 14 * 16 -> 26880

Both output are greater than the max value for int8 (127).
So the output should be 127, 127.

I have the same wrong result with my own QuantizedProd implementation and the QuantizedReduceProd code already present in the repo. sorry I am not very familiar with Quantized operation.

Did I missunderstand the quantized process?

@rascani
Copy link
Contributor

rascani commented Apr 2, 2024

Should I used this existing function? Should I remove it and implement a new one based on the SumOrMean implementation?

I would suggest re-using the QuantizedReduceProd in the reduce.h header.

I'll take a look at the test in the PR.

@Evanok
Copy link
Author

Evanok commented Apr 2, 2024

Hello @rascani. I just pushed into the PR branch the problematic test. This is going to be easier to reproduce the issue!
I am going to try on my side to find documentation on Quantization. I feel like the real problem is a lack of understanding on my side.

Copy link
Contributor

"This issue is being marked as stale due to inactivity. Remove label or comment to prevent closure in 5 days."

@github-actions github-actions bot added the Stale label May 22, 2024
Copy link
Contributor

"This issue is being closed because it has been marked as
stale for 5 days with no further activity."

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants