Resizing Layer #6879

koyykdy · 2022-09-29T18:50:59Z

The branch and commit histories have been cleaned up for the PR.

IMPORT NOTE:
The lower-level op implementation of image resizing-nearest neighbor in TensorFlow.js differs from the implementation of the comparable op in Keras-Python.

While the Python version of the op function always selects the bottom right cell of the sub-matrix to be used as the representative value of that region in the downscaled matrix, the JavaScript implementation defaults to the top left cell of the sub-matrix, and then preferentially shifts to the right side of the sub-matrix in all sub-matrices past the lateral halfway point ( calculated by floor((length-1)/2) ), and the bottom side of the sub-matrix in all sub-matrices past the vertical halfway point, when considering the top-left side of the parent matrix as the origin.

This causes a slight variation in the output values from nearest neighbor downscaling between the Python and JavaScript versions of the code as it currently stands, and the unit tests for the resizing layer has been implemented to reflect this difference in op-function behavior.

Co-authored-by: Adam Lang (@AdamLang96) adamglang96@gmail.com
Co-authored-by: Brian Zheng (@Brianzheng123) brianzheng345@gmail.com

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.

This change is

* Update jasmine_util.ts (tensorflow#6872) FIX * webgl: Fix NaN issue (tensorflow#6828) Fix tensorflow#6822 Problem 1： On some GPUs, even if a and b are both non-NaN, the value of isNaN in vec4 isNaN = min(vec4(isnan(a)) + vec4(isnan(b)), vec4(1.0)); are still larger than 0., which misleads all values become NAN. 2： After resolving NAN issue, the result is still incorrect. It seems that the isnan_custom is not well supported on the problem GPU. After switching back to builtin isnan, everything works well. Solution: Use the bool type bvec4 instead of float type vec4 to calculate isNaN to avoid the the float precision issue when comparing with zero. Meanwhile, add an env flag WEBGL2_ISNAN_CUSTOM to allow user to specify which isnan to use. * Upgrade nodejs to 18.7.0 (tensorflow#6863) * Upgrade nodejs to 18.7.0 * Fix hash table test string not passed as base64 * fixed prelu fusing code that pre-maturely neg the const on multiply (tensorflow#6876) Co-authored-by: RajeshT <43972606+rthadur@users.noreply.github.com> Co-authored-by: Linchenn <40653845+Linchenn@users.noreply.github.com> Co-authored-by: Jiajia Qin <jiajia.qin@intel.com> Co-authored-by: Matthew Soulanille <msoulanille@google.com> Co-authored-by: Ping Yu <4018+pyu10055@users.noreply.github.com> Co-authored-by: RajeshT <43972606+rthadur@users.noreply.github.com>

@AdamLang96

* Implementation of preprocessing image resizing layer Important Note: The lower-level op implementation of image resizing-nearest neighbor in TensorFlow.js differs from the implementation of the comparable op in Keras-Python. While the Python version of the op function always selects the bottom right cell of the sub-matrix to be used as the representative value of that region in the downscaled matrix, the JavaScript implementation defaults to the top left cell of the sub-matrix, and then preferentially shifts to the right side of the sub-matrix in all sub-matrices past the lateral halfway point ( calculated by floor((length-1)/2) ), and the bottom side of the sub-matrix in all sub-matrices past the vertical halfway point, when considering the top-left side of the parent matrix as the origin. This causes a slight variation in the output values from nearest neighbor downscaling between the Python and JavaScript versions of the code as it currently stands, and the unit tests for the resizing layer has been implemented to reflect this difference in op-function behavior. Co-authored-by: Adam Lang (@AdamLang96) <adamglang96@gmail.com> Co-authored-by: Brian Zheng (@Brianzheng123) <brianzheng345@gmail.com> * Updating image preprocessing test file to include approved usage of CodeSmith licensing header Co-authored-by: Adam Lang (@AdamLang96) <adamglang96@gmail.com> Co-authored-by: Brian Zheng (@Brianzheng123) <brianzheng345@gmail.com>

* Update jasmine_util.ts (tensorflow#6872) FIX * webgl: Fix NaN issue (tensorflow#6828) Fix tensorflow#6822 Problem 1： On some GPUs, even if a and b are both non-NaN, the value of isNaN in vec4 isNaN = min(vec4(isnan(a)) + vec4(isnan(b)), vec4(1.0)); are still larger than 0., which misleads all values become NAN. 2： After resolving NAN issue, the result is still incorrect. It seems that the isnan_custom is not well supported on the problem GPU. After switching back to builtin isnan, everything works well. Solution: Use the bool type bvec4 instead of float type vec4 to calculate isNaN to avoid the the float precision issue when comparing with zero. Meanwhile, add an env flag WEBGL2_ISNAN_CUSTOM to allow user to specify which isnan to use. * Upgrade nodejs to 18.7.0 (tensorflow#6863) * Upgrade nodejs to 18.7.0 * Fix hash table test string not passed as base64 * fixed prelu fusing code that pre-maturely neg the const on multiply (tensorflow#6876) Co-authored-by: RajeshT <43972606+rthadur@users.noreply.github.com> Co-authored-by: Linchenn <40653845+Linchenn@users.noreply.github.com> Co-authored-by: Jiajia Qin <jiajia.qin@intel.com> Co-authored-by: Matthew Soulanille <msoulanille@google.com> Co-authored-by: Ping Yu <4018+pyu10055@users.noreply.github.com> Co-authored-by: RajeshT <43972606+rthadur@users.noreply.github.com>

koyykdy · 2022-09-29T18:52:56Z

@ahmedsabie @mattsoulanille Suggestions have been implemented, requesting review. Thank you!

mattsoulanille · 2022-09-29T23:17:02Z

This new PR has changes from packages other than tfjs-layers. Is that intentional?

We don't mind PRs having a messy commit history since we squash all commits into a single one when merging. Can we re-open your original PR #6858? This also helps me review your code since I can see changes since my last review.

If you want help with git history (e.g. getting just your changes in the PR), I can reset the resizing branch so it includes only files from tfjs-layers.

koyykdy · 2022-09-30T01:23:42Z

This new PR has changes from packages other than tfjs-layers. Is that intentional?

The only changes aside from the tfjs-layers/src/layers/preprocessing should be in tfjs-layers/src/exports-layers.ts for registering the new layer. Any other changes would be from pulling in the new commits made by third parties to the tfjs master repository late last evening and this morning, which made the base branch for this PR outdated.

We don't mind PRs having a messy commit history since we squash all commits into a single one when merging. Can we re-open your original PR #6858? This also helps me review your code since I can see changes since my last review.

If you want help with git history (e.g. getting just your changes in the PR), I can reset the resizing branch so it includes only files from tfjs-layers.

There was a typo made to one of the commits in that branch where the Co-authored-by commit field had incorrectly applied brackets to the github username of a co-author instead of their e-mail address, which was causing the automatic CI tool to fail during checking. I deleted the forks and branches and recreated this one in order to address that issue.

koyykdy · 2022-09-30T01:29:10Z

The only changes aside from the tfjs-layers/src/layers/preprocessing should be in tfjs-layers/src/exports-layers.ts for registering the new layer. Any other changes would be from pulling in the new commits made by third parties to the tfjs master repository late last evening and this morning, which made the base branch for this PR outdated.

These are the relevant commits in the pull request:
These commits are pulling in changes to the master branch to silence This branch is out-of-date with the base branch warning in GitHub:
CodeSmithDSMLProjects#41
CodeSmithDSMLProjects#43
Pull 43 is effectively the same as pull 41, I accidentally did it twice because doing it the first time did not make the This branch is out-of-date with the base branch warning go away.

This is the only commit containing the resizing layer changes:
CodeSmithDSMLProjects#42

mattsoulanille

LGTM. Thanks!

mattsoulanille · 2022-10-04T16:55:12Z

@ahmedsabie Please take a look at this PR when you get a chance. Thanks!

tfjs-layers/src/layers/preprocessing/image_resizing.ts

@AdamLang96

* Implementation of preprocessing image resizing layer Important Note: The lower-level op implementation of image resizing-nearest neighbor in TensorFlow.js differs from the implementation of the comparable op in Keras-Python. While the Python version of the op function always selects the bottom right cell of the sub-matrix to be used as the representative value of that region in the downscaled matrix, the JavaScript implementation defaults to the top left cell of the sub-matrix, and then preferentially shifts to the right side of the sub-matrix in all sub-matrices past the lateral halfway point ( calculated by floor((length-1)/2) ), and the bottom side of the sub-matrix in all sub-matrices past the vertical halfway point, when considering the top-left side of the parent matrix as the origin. This causes a slight variation in the output values from nearest neighbor downscaling between the Python and JavaScript versions of the code as it currently stands, and the unit tests for the resizing layer has been implemented to reflect this difference in op-function behavior. Co-authored-by: Adam Lang (@AdamLang96) <adamglang96@gmail.com> Co-authored-by: Brian Zheng (@Brianzheng123) <brianzheng345@gmail.com> * Updating image preprocessing test file to include approved usage of CodeSmith licensing header * refactored interpolation type declaration Co-authored-by: Adam Lang (@AdamLang96) <adamglang96@gmail.com> Co-authored-by: Brian Zheng (@Brianzheng123) <brianzheng345@gmail.com>

koyykdy · 2022-10-06T21:24:42Z

@ahmedsabie Hello, the build is failing for an unknown reason, could you check it over if you can?

@AdamLang96

* Implementation of preprocessing image resizing layer Important Note: The lower-level op implementation of image resizing-nearest neighbor in TensorFlow.js differs from the implementation of the comparable op in Keras-Python. While the Python version of the op function always selects the bottom right cell of the sub-matrix to be used as the representative value of that region in the downscaled matrix, the JavaScript implementation defaults to the top left cell of the sub-matrix, and then preferentially shifts to the right side of the sub-matrix in all sub-matrices past the lateral halfway point ( calculated by floor((length-1)/2) ), and the bottom side of the sub-matrix in all sub-matrices past the vertical halfway point, when considering the top-left side of the parent matrix as the origin. This causes a slight variation in the output values from nearest neighbor downscaling between the Python and JavaScript versions of the code as it currently stands, and the unit tests for the resizing layer has been implemented to reflect this difference in op-function behavior. Co-authored-by: Adam Lang (@AdamLang96) <adamglang96@gmail.com> Co-authored-by: Brian Zheng (@Brianzheng123) <brianzheng345@gmail.com> * Updating image preprocessing test file to include approved usage of CodeSmith licensing header * refactored interpolation type declaration * minor spacing correction Co-authored-by: Adam Lang (@AdamLang96) <adamglang96@gmail.com> Co-authored-by: Brian Zheng (@Brianzheng123) <brianzheng345@gmail.com>

mattsoulanille · 2022-10-06T23:43:31Z

@koyykdy Sorry about that build failure. We had changed some cloudbuild permissions on our CI runner, and it broke presubmits. It should be fixed now, though.

koyykdy · 2022-10-06T23:56:50Z

@mattsoulanille It seems to be failing still, could you please check what the output from the presubmit failure is? Thank you.

mattsoulanille · 2022-10-07T19:14:57Z

Looks like it failed to compile:

tfjs-layers/src/exports_layers.ts:1803:17 - error TS6133: 'categoryEncoding' is declared but its value is never read.

1803 export function categoryEncoding(args: CategoryEncodingArgs) {
                     ~~~~~~~~~~~~~~~~

That's strange because usually exporting a value makes typescript not try to check if it's used.

koyykdy · 2022-10-07T19:16:58Z

Looks like it failed to compile:
tfjs-layers/src/exports_layers.ts:1803:17 - error TS6133: 'categoryEncoding' is declared but its value is never read.

1803 export function categoryEncoding(args: CategoryEncodingArgs) {
                     ~~~~~~~~~~~~~~~~
That's strange because usually exporting a value makes typescript not try to check if it's used.

This seems to be from the updates brought in by merging in from the master branch, could there be an issue with regards to this?

koyykdy · 2022-10-07T19:17:52Z

I attempted to update the branch, and resolved a merge conflict in exports_layers, but the rest of the changes from the main branch merge request might not have gone through.

mattsoulanille · 2022-10-07T19:23:22Z

Fixed it. Missing a close curly brace.

mattsoulanille · 2022-10-07T19:25:22Z

The build succeeds now, but the tests still have a compilation error:

tfjs-layers/src/layers/preprocessing/image_resizing_test.ts:113:31 - error TS2345: Argument of type '{ height: number; width: number; interpolation: string; }' is not assignable to parameter of type 'ResizingArgs'.
  Types of property 'interpolation' are incompatible.
    Type 'string' is not assignable to type 'InterpolationFormat'.

113     expect(() => new Resizing(incorrectArgs)).toThrowError(expectedError);
                                  ~~~~~~~~~~~~~

tfjs-layers/src/layers/preprocessing/image_resizing_test.ts

Co-authored-by: Matthew Soulanille <matthew@soulanille.net>

@AdamLang96

…57) * Implementation of preprocessing image resizing layer Important Note: The lower-level op implementation of image resizing-nearest neighbor in TensorFlow.js differs from the implementation of the comparable op in Keras-Python. While the Python version of the op function always selects the bottom right cell of the sub-matrix to be used as the representative value of that region in the downscaled matrix, the JavaScript implementation defaults to the top left cell of the sub-matrix, and then preferentially shifts to the right side of the sub-matrix in all sub-matrices past the lateral halfway point ( calculated by floor((length-1)/2) ), and the bottom side of the sub-matrix in all sub-matrices past the vertical halfway point, when considering the top-left side of the parent matrix as the origin. This causes a slight variation in the output values from nearest neighbor downscaling between the Python and JavaScript versions of the code as it currently stands, and the unit tests for the resizing layer has been implemented to reflect this difference in op-function behavior. Co-authored-by: Adam Lang (@AdamLang96) <adamglang96@gmail.com> Co-authored-by: Brian Zheng (@Brianzheng123) <brianzheng345@gmail.com> * Updating image preprocessing test file to include approved usage of CodeSmith licensing header * refactored interpolation type declaration * minor spacing correction * resizing layer test assertion statement argument casting refactored Co-authored-by: Adam Lang (@AdamLang96) <adamglang96@gmail.com> Co-authored-by: Brian Zheng (@Brianzheng123) <brianzheng345@gmail.com>

@AdamLang96

* Implementation of preprocessing image resizing layer Important Note: The lower-level op implementation of image resizing-nearest neighbor in TensorFlow.js differs from the implementation of the comparable op in Keras-Python. While the Python version of the op function always selects the bottom right cell of the sub-matrix to be used as the representative value of that region in the downscaled matrix, the JavaScript implementation defaults to the top left cell of the sub-matrix, and then preferentially shifts to the right side of the sub-matrix in all sub-matrices past the lateral halfway point ( calculated by floor((length-1)/2) ), and the bottom side of the sub-matrix in all sub-matrices past the vertical halfway point, when considering the top-left side of the parent matrix as the origin. This causes a slight variation in the output values from nearest neighbor downscaling between the Python and JavaScript versions of the code as it currently stands, and the unit tests for the resizing layer has been implemented to reflect this difference in op-function behavior. Co-authored-by: Adam Lang (@AdamLang96) <adamglang96@gmail.com> Co-authored-by: Brian Zheng (@Brianzheng123) <brianzheng345@gmail.com> * Updating image preprocessing test file to include approved usage of CodeSmith licensing header * refactored interpolation type declaration * minor spacing correction * resizing layer test assertion statement argument casting refactored * image resizing layer unit test assertion line max length exceeded refactored Co-authored-by: Adam Lang (@AdamLang96) <adamglang96@gmail.com> Co-authored-by: Brian Zheng (@Brianzheng123) <brianzheng345@gmail.com>

mattsoulanille · 2022-10-10T17:51:10Z

@ahmedsabie Please take another look when you get a chance. Thanks!

koyykdy · 2022-12-22T17:41:27Z

Hello, I do not currently show up under contributors to the tfjs repository on GitHub, is there a way to address this?

mattsoulanille · 2022-12-22T18:03:49Z

I'm out of office at the moment, so I'll refer you to @Linchenn.

Linchenn · 2022-12-22T18:35:31Z

I think you are a contributor since you have merged a commit to our default branch. Probably you are not displayed in the contributor list. https://docs.github.com/en/repositories/viewing-activity-and-data-for-your-repository/viewing-a-projects-contributors @koyykdy

koyykdy and others added 3 commits September 29, 2022 11:10

koyykdy changed the title ~~Resizing Layer Implemented and Refactored~~ Resizing Layer Sep 29, 2022

mattsoulanille self-requested a review September 29, 2022 21:55

Merge branch 'master' into resizing

807fedf

mattsoulanille approved these changes Sep 30, 2022

View reviewed changes

mattsoulanille requested a review from ahmedsabie September 30, 2022 17:53

ahmedsabie suggested changes Oct 6, 2022

View reviewed changes

tfjs-layers/src/layers/preprocessing/image_resizing.ts Outdated Show resolved Hide resolved

koyykdy and others added 3 commits October 6, 2022 14:29

Merge branch 'master' into resizing

05d0837

Merge branch 'master' into resizing

5e57be4

mattsoulanille requested a review from ahmedsabie October 6, 2022 23:46

Add missing '}' to exports_layers.ts

e90cc76

mattsoulanille reviewed Oct 7, 2022

View reviewed changes

tfjs-layers/src/layers/preprocessing/image_resizing_test.ts Outdated Show resolved Hide resolved

Update tfjs-layers/src/layers/preprocessing/image_resizing_test.ts

e16cace

Co-authored-by: Matthew Soulanille <matthew@soulanille.net>

koyykdy and others added 2 commits October 7, 2022 13:16

ahmedsabie approved these changes Oct 11, 2022

View reviewed changes

Merge branch 'master' into resizing

d0bdcdb

mattsoulanille merged commit 9752734 into tensorflow:master Oct 11, 2022

mattsoulanille deleted the resizing branch October 11, 2022 20:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resizing Layer #6879

Resizing Layer #6879

koyykdy commented Sep 29, 2022 •

edited by nsthorat

koyykdy commented Sep 29, 2022

mattsoulanille commented Sep 29, 2022

koyykdy commented Sep 30, 2022

koyykdy commented Sep 30, 2022 •

edited

mattsoulanille left a comment

mattsoulanille commented Oct 4, 2022

koyykdy commented Oct 6, 2022

mattsoulanille commented Oct 6, 2022

koyykdy commented Oct 6, 2022

mattsoulanille commented Oct 7, 2022

koyykdy commented Oct 7, 2022

koyykdy commented Oct 7, 2022

mattsoulanille commented Oct 7, 2022

mattsoulanille commented Oct 7, 2022

mattsoulanille commented Oct 10, 2022

koyykdy commented Dec 22, 2022

mattsoulanille commented Dec 22, 2022

Linchenn commented Dec 22, 2022

Resizing Layer #6879

Resizing Layer #6879

Conversation

koyykdy commented Sep 29, 2022 • edited by nsthorat

koyykdy commented Sep 29, 2022

mattsoulanille commented Sep 29, 2022

koyykdy commented Sep 30, 2022

koyykdy commented Sep 30, 2022 • edited

mattsoulanille left a comment

Choose a reason for hiding this comment

mattsoulanille commented Oct 4, 2022

koyykdy commented Oct 6, 2022

mattsoulanille commented Oct 6, 2022

koyykdy commented Oct 6, 2022

mattsoulanille commented Oct 7, 2022

koyykdy commented Oct 7, 2022

koyykdy commented Oct 7, 2022

mattsoulanille commented Oct 7, 2022

mattsoulanille commented Oct 7, 2022

mattsoulanille commented Oct 10, 2022

koyykdy commented Dec 22, 2022

mattsoulanille commented Dec 22, 2022

Linchenn commented Dec 22, 2022

koyykdy commented Sep 29, 2022 •

edited by nsthorat

koyykdy commented Sep 30, 2022 •

edited