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

Support RESIZE_BILINEAR in TFLu #43426

Merged
merged 16 commits into from Apr 12, 2021

Conversation

patriklaurell
Copy link
Contributor

@patriklaurell patriklaurell commented Sep 21, 2020

Adds support for RESIZE_BILINEAR in TFLu. This PR is related to this issue b/168339972. It depends on the flatbuffer conversion changes in this PR, which were submitted as a separate PR as suggested by @advaitjain, and is not relevant for review until that PR has been merged.

@google-ml-butler google-ml-butler bot added the size:XL CL Change Size:Extra Large label Sep 21, 2020
@google-ml-butler
Copy link

Thanks for contributing to TensorFlow Lite Micro.

To keep this process moving along, we'd like to make sure that you have completed the items on this list:

We would like to have a discussion on the Github issue first to determine the best path forward, and then proceed to the PR review.

@patriklaurell patriklaurell marked this pull request as draft September 21, 2020 14:49
@gbaned gbaned self-assigned this Sep 22, 2020
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Sep 22, 2020
@patriklaurell patriklaurell marked this pull request as ready for review September 25, 2020 13:33
@gbaned gbaned added comp:lite TF Lite related issues comp:micro Related to TensorFlow Lite Microcontrollers labels Sep 25, 2020
@gbaned gbaned added the awaiting review Pull request awaiting review label Sep 30, 2020
Copy link
Member

@advaitjain advaitjain left a comment

Choose a reason for hiding this comment

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

Apologies for the long delay here.

The review is half-baked at this time and I'm realizing that there are many things that we should put down into a porting guide. I'm going to take some time to make that happen, which unfortunately means some more delay on this particular review.

Thanks for your patience as we improve our documentation.

No action needed at this time from you. I'll put down a proper guide and then respond to this PR.

Comment on lines +17 to +18
float scaled_value_floor = std::floor(*scaled_value);
*lower_bound = std::max(static_cast<int32_t>(scaled_value_floor),
Copy link
Member

Choose a reason for hiding this comment

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

#include <algorithm>
#include <cmath>
#include <cstdint>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I understood this comment correctly. I included these includes at the top of the file. Was that what you had in mind?

@@ -0,0 +1,199 @@
#include "tensorflow/lite/kernels/internal/cppmath.h"
Copy link
Member

Choose a reason for hiding this comment

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

One request would be to branch this from reference_ops.h so that it is clear exactly what (if any changes are made to the reference implementation).

git cp reference_ops.h resize_bilinear.h

and then remove all the extra code. The idea is to make it clear that the only operation was the code being copied, and nothing else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't seem to find the git cp command. I don't know how to accomplish what you are describing. How would you do it without using git cp? @advaitjain

tensorflow/lite/micro/kernels/resize_bilinear.cc Outdated Show resolved Hide resolved
tensorflow/lite/micro/kernels/resize_bilinear.cc Outdated Show resolved Hide resolved
tensorflow/lite/micro/kernels/resize_bilinear.cc Outdated Show resolved Hide resolved
tensorflow/lite/micro/kernels/resize_bilinear.cc Outdated Show resolved Hide resolved
@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Oct 10, 2020
@gbaned
Copy link
Contributor

gbaned commented Oct 14, 2020

@patriklaurell Can you please check @advaitjain's comments and keep us posted ? Thanks!

@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label Oct 14, 2020
@tensorflowbutler tensorflowbutler removed the stat:awaiting response Status - Awaiting response from author label Oct 21, 2020
@gbaned gbaned added the awaiting review Pull request awaiting review label Oct 23, 2020
@gbaned
Copy link
Contributor

gbaned commented Nov 3, 2020

@patriklaurell Can you please resolve conflicts? Thanks!

@gbaned gbaned added stat:awaiting response Status - Awaiting response from author and removed awaiting review Pull request awaiting review labels Nov 3, 2020
@advaitjain
Copy link
Member

@patriklaurell Can you please resolve conflicts? Thanks!

@patriklaurell no need for any changes. Its on my list of things to address this PR but let's keep this on ice for now.

@gbaned gbaned added stat:awaiting tensorflower Status - Awaiting response from tensorflower and removed stat:awaiting response Status - Awaiting response from author labels Nov 4, 2020
@gbaned gbaned added the awaiting review Pull request awaiting review label Mar 17, 2021
Copy link
Member

@advaitjain advaitjain left a comment

Choose a reason for hiding this comment

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

quick approve for small changes (since @petewarden has already approved the changes)

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 22, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 22, 2021
@gbaned gbaned added ready to pull PR ready for merge process and removed awaiting review Pull request awaiting review ready to pull PR ready for merge process labels Mar 23, 2021
@patriklaurell
Copy link
Contributor Author

@petewarden @advaitjain sorry for the failing test. I am looking into it now. Will push the fix as soon as possible.

@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Mar 26, 2021
@gbaned gbaned requested a review from advaitjain March 26, 2021 14:31
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 26, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 26, 2021
@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Apr 12, 2021
@advaitjain advaitjain added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Apr 12, 2021
@advaitjain
Copy link
Member

Internal checks seemed to be running into merge conflicts. I have pushed a commit that updates this PR to tip of tree. My assumption is that we should now be able to get this merged.

@copybara-service copybara-service bot merged commit dfd36cf into tensorflow:master Apr 12, 2021
PR Queue automation moved this from Approved by Reviewer to Merged Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes comp:lite TF Lite related issues comp:micro Related to TensorFlow Lite Microcontrollers kokoro:force-run Tests on submitted change ready to pull PR ready for merge process size:XL CL Change Size:Extra Large
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

8 participants