Skip to content

Conversation

wchargin
Copy link
Contributor

@wchargin wchargin commented Dec 9, 2019

Summary:
The images plugin backend sends down images’ widths and heights, but the
frontend does not actually need or use this data. (A comment claiming
otherwise was in error.) This commit removes that behavior.

Test Plan:
The dashboard behavior appears unchanged, with both fixed-width and
actual-size display behaviors at both the per-image and dashboard-global
levels. All references to width or height in modified files are now
irrelevant. No network responses now contain width or height.

wchargin-branch: images-no-dims

Summary:
The images plugin backend sends down images’ widths and heights, but the
frontend does not actually need or use this data. (A comment claiming
otherwise was in error.) This commit removes that behavior.

Test Plan:
The dashboard behavior appears unchanged, with both fixed-width and
actual-size display behaviors at both the per-image and dashboard-global
levels. All references to `width` or `height` in modified files are now
irrelevant. No network responses now contain `width` or `height`.

wchargin-branch: images-no-dims
wchargin-branch: images-no-dims
wchargin-source: 107104491a31e9b6dd5a03fdeaf195dc20513631
Copy link
Contributor

@stephanwlee stephanwlee left a comment

Choose a reason for hiding this comment

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

One thing to consider: often on the frontend, it is better to know the dimensions upfront without fetching the data for tiling or pre-allocating box for the layout. As you indicated, we do not make use of that right now but we may want it.

wchargin-branch: images-no-dims
wchargin-source: e63d04fabba6b1015d24280ef65ffbbf6a0ed1b2

# Conflicts:
#	tensorboard/plugins/image/images_plugin.py
#	tensorboard/plugins/image/images_plugin_test.py
wchargin-branch: images-no-dims
wchargin-source: e63d04fabba6b1015d24280ef65ffbbf6a0ed1b2
@wchargin
Copy link
Contributor Author

wchargin commented Jan 2, 2020

One thing to consider: often on the frontend, it is better to know the
dimensions upfront without fetching the data for tiling or
pre-allocating box for the layout. As you indicated, we do not make
use of that right now but we may want it.

Thanks: I appreciate you calling this out, as I was indeed considering
dropping it, but you’re probably right that we should keep it around.
I think that I’ll go ahead and merge this, as the change is still valid
and useful, but will retain the actual width and height data for now.

@wchargin wchargin merged commit 6610102 into master Jan 2, 2020
@wchargin wchargin deleted the wchargin-images-no-dims branch January 2, 2020 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants