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

[image]: Add a version for readPngImageAndPreprocess that can also take a tensor as input. #2896

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@ZchiPitt
Copy link
Contributor

commented May 13, 2019

Description:
Implement a version for readPngImageAndPreprocess that can also take a tensor as input.
Testing:
Add a test case in ImageTest: readPngImageAndPreprocessWithAndWithoutInputTensor() that verifies the new version works correctly.
[Optional Fixes #issue]
#2358

@ZchiPitt ZchiPitt requested review from nickgg and opti-mix May 13, 2019

@ZchiPitt ZchiPitt requested review from SplitInfinity and jfix71 May 13, 2019

@arunm-git
Copy link
Contributor

left a comment

Changes look good.

Nit: The doxygen comments are easier to read when each \param is on a new line. Can we change that?

@nickgg

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

Added #2898 to cover porting the resnet example code to this new loader function.

Show resolved Hide resolved lib/Base/Image.cpp Outdated
Show resolved Hide resolved include/glow/Base/Image.h Outdated
Show resolved Hide resolved include/glow/Base/Image.h Outdated
Show resolved Hide resolved include/glow/Base/Image.h Outdated
Show resolved Hide resolved tests/unittests/ImageTest.cpp Outdated
size_t imgWidth = image1.dims()[1];
size_t numChannels = image1.dims()[2];

Tensor swizzled(image1.getType());

This comment has been minimized.

Copy link
@SplitInfinity

SplitInfinity May 13, 2019

Contributor

This code code seems to be identical to the preprocessing code in glow::readPngImageAndPreprocessHelper. I'm not sure what the value of this is because a bug in both places won't get caught and a bug that is introduced into readPngImageAndPreprocess is likely to be hidden by copying that code here whenever it is updated.

This comment has been minimized.

Copy link
@ZchiPitt

ZchiPitt May 14, 2019

Author Contributor

sounds right. I modified the test case that I move the transpose operation on image2 instead to make image2 become NHWC and do the RBG change to image1, then i can check whether image1 equals image2.

It should be same logic but the operations are not same as the readPngImageAndPreprocess() implementation

@ZchiPitt

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Changes look good.

Nit: The doxygen comments are easier to read when each \param is on a new line. Can we change that?

@arunm-git sure. It must have been because of the auto format script lol

@ZchiPitt ZchiPitt force-pushed the ZchiPitt:readPngImage branch 3 times, most recently from 5b4375f to 1803d7a May 14, 2019

@facebook-github-bot
Copy link

left a comment

@ZchiPitt has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Add a version for readPngImageAndPreprocess that can also take a tens…
…or as input. (#2896)

Summary:
*Description*:
Implement a version for readPngImageAndPreprocess that can also take a tensor as input.
*Testing*:
Add a test case in ImageTest: readPngImageAndPreprocessWithAndWithoutInputTensor() that verifies the new version works correctly.
[Optional Fixes #issue]
Pull Request resolved: #2896

Differential Revision: D15343817

fbshipit-source-id: b876cbaa062c35e2f297902b1cd4d9871cfae9e0

@ZchiPitt ZchiPitt force-pushed the ZchiPitt:readPngImage branch from 1803d7a to 7a9c651 May 14, 2019

@ZchiPitt ZchiPitt requested a review from SplitInfinity May 15, 2019

@facebook-github-bot
Copy link

left a comment

@ZchiPitt is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot

This comment has been minimized.

Copy link

commented May 16, 2019

@ZchiPitt merged this pull request in 818cba2.

@ZchiPitt ZchiPitt deleted the ZchiPitt:readPngImage branch May 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.