KEMBAR78
Fix reverse order of w & h by ppwwyyxx · Pull Request #1262 · tensorflow/tensorboard · GitHub
Skip to content

Fix reverse order of w & h #1262

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 16, 2018
Merged

Fix reverse order of w & h #1262

merged 3 commits into from
Jul 16, 2018

Conversation

@ppwwyyxx
Copy link
Contributor

HxWxC tensor is what's commonly used to represent images.
Also this function calls util.encode_png which expects HxWxC tensor.

@ppwwyyxx
Fix reverse order of w & h
b292fd1 
HxWxC tensor is what's commonly used to represent images.
Also this function calls `util.encode_png` which expects HxWxC tensor.
@nfelt
Copy link
Contributor

nfelt commented Jul 4, 2018 •
edited
Loading

Thanks for catching this, it definitely should be HxWxC.

Would you mind doing the same changes to the op() version? It has the same issue, and there's also a compatibility test that checks that the pb() and op() methods produce identical output, so that test currently doesn't pass with this change. A few of the tests also currently have the incorrect width-then-height ordering, so they will also need to be updated.

I think fixing the tests should be as straightforward as swapping "width" and "height" on these three lines:
https://github.com/ppwwyyxx/tensorboard/blob/b292fd1373e75027e16b99bedf601bf3640b369d/tensorboard/plugins/image/summary_test.py#L44
https://github.com/ppwwyyxx/tensorboard/blob/b292fd1373e75027e16b99bedf601bf3640b369d/tensorboard/plugins/image/summary_test.py#L80
https://github.com/ppwwyyxx/tensorboard/blob/b292fd1373e75027e16b99bedf601bf3640b369d/tensorboard/plugins/image/summary_test.py#L141

@nfelt nfelt self-requested a review July 4, 2018 02:09
@ppwwyyxx
Copy link
Contributor Author

@nfelt The tests have passed! Sorry for the delay.

nfelt
nfelt approved these changes Jul 16, 2018
View reviewed changes
Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks again for noticing and sending the fix!

@nfelt nfelt merged commit 06069bb into tensorflow:master Jul 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@nfelt nfelt nfelt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

plugin:images

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ppwwyyxx @nfelt