-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Resizing Layer #6879
Resizing Layer #6879
Conversation
* 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 <[email protected]> Co-authored-by: Linchenn <[email protected]> Co-authored-by: Jiajia Qin <[email protected]> Co-authored-by: Matthew Soulanille <[email protected]> Co-authored-by: Ping Yu <[email protected]> Co-authored-by: RajeshT <[email protected]>
* 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) <[email protected]> Co-authored-by: Brian Zheng (@Brianzheng123) <[email protected]> * Updating image preprocessing test file to include approved usage of CodeSmith licensing header Co-authored-by: Adam Lang (@AdamLang96) <[email protected]> Co-authored-by: Brian Zheng (@Brianzheng123) <[email protected]>
* 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 <[email protected]> Co-authored-by: Linchenn <[email protected]> Co-authored-by: Jiajia Qin <[email protected]> Co-authored-by: Matthew Soulanille <[email protected]> Co-authored-by: Ping Yu <[email protected]> Co-authored-by: RajeshT <[email protected]>
@ahmedsabie @mattsoulanille Suggestions have been implemented, requesting review. Thank you! |
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 |
The only changes aside from the
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. |
These are the relevant commits in the pull request: This is the only commit containing the resizing layer 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.
LGTM. Thanks!
@ahmedsabie Please take a look at this PR when you get a chance. Thanks! |
* 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) <[email protected]> Co-authored-by: Brian Zheng (@Brianzheng123) <[email protected]> * Updating image preprocessing test file to include approved usage of CodeSmith licensing header * refactored interpolation type declaration Co-authored-by: Adam Lang (@AdamLang96) <[email protected]> Co-authored-by: Brian Zheng (@Brianzheng123) <[email protected]>
@ahmedsabie Hello, the build is failing for an unknown reason, could you check it over if you can? |
* 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) <[email protected]> Co-authored-by: Brian Zheng (@Brianzheng123) <[email protected]> * 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) <[email protected]> Co-authored-by: Brian Zheng (@Brianzheng123) <[email protected]>
@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. |
@mattsoulanille It seems to be failing still, could you please check what the output from the presubmit failure is? Thank you. |
Looks like it failed to compile:
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? |
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. |
Fixed it. Missing a close curly brace. |
The build succeeds now, but the tests still have a compilation error:
|
Co-authored-by: Matthew Soulanille <[email protected]>
…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) <[email protected]> Co-authored-by: Brian Zheng (@Brianzheng123) <[email protected]> * 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) <[email protected]> Co-authored-by: Brian Zheng (@Brianzheng123) <[email protected]>
* 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) <[email protected]> Co-authored-by: Brian Zheng (@Brianzheng123) <[email protected]> * 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) <[email protected]> Co-authored-by: Brian Zheng (@Brianzheng123) <[email protected]>
@ahmedsabie Please take another look when you get a chance. Thanks! |
Hello, I do not currently show up under contributors to the tfjs repository on GitHub, is there a way to address this? |
I'm out of office at the moment, so I'll refer you to @Linchenn. |
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 |
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) [email protected]
Co-authored-by: Brian Zheng (@Brianzheng123) [email protected]
To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is