-
Notifications
You must be signed in to change notification settings - Fork 954
Porting Dynamic_Update_Slice operator from TFLite #3246
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
base: main
Are you sure you want to change the base?
Conversation
Sync changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks! I just have a few small comments to address.
| } | ||
| } | ||
|
|
||
| // TODO(rameshkunasi): Add quantized test for dynamic update slice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you planning to add this in the separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for finding this. For this kernel quantized test is not required. Added remaining test
|
|
||
| // min/max are used to compute scale, zero-point is 0 | ||
| template <size_t kInputSize> | ||
| struct TestDynamicUpdateSliceParams { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you use this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not useful. Removed
| namespace testing { | ||
| namespace { | ||
|
|
||
| // constexpr float kTestTolerance = 7.41e-03; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not required. Removed
| kInputDims_0, kInputDims_1, kInputDims_2}; | ||
| int kOutputDims[] = {2, 3, 3}; | ||
|
|
||
| constexpr float kInput_0[] = {1, 2, 3, 4, 5, 6, 7, 8, 9}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I missed something here; The dimension for this is defined {2, 3, 3} above but the array here only has 9 elements? Where is the remaining half?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@veblush kOutputDims is a stand-in array for TfLiteIntArray. The first element of the array is the number of array elements. This occurs throughout the unit tests (using IntArrayFromInts). TfLiteFloatArray is even hairier as the first element of the float array has to be rewritten as an int by FloatArrayFromFloats...
| TF_LITE_ENSURE(context, | ||
| SizeOfDimension(update, i) <= SizeOfDimension(operand, i)); | ||
| } | ||
| output->type = operand->type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious what's the purpose of this? Output tensor will be deallocated right away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rameshkunasi I am also curious as to why the output tensor type is not already set by the TfLiteConverter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pointing this. Not required, removed this
| output_data); | ||
| } | ||
|
|
||
| TF_LITE_MICRO_TEST(DynamicUpdateSliceOpTestBoundaryTest) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of this test? How is this different from DynamicUpdateSliceOpTestSimpleFloat?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is same as DynamicUpdateSliceOpTestSimpleFloat except the indices. Here in this test indices used are corner case
| if (current_dim == max_dims) { | ||
| return kTfLiteOk; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is unreachable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will, because it will be called by increasing current_dim count in recursively
Added Dynamic_Update_Slice operator changes.
BUG=fixes #3217