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

Example adding various images #490

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

gar1t
Copy link
Contributor

@gar1t gar1t commented Aug 17, 2019

This is inspired by #21, which wants a way to easily add a PNG as an image summary. The examples demonstrate how to do this.

FWIW I think this example is a very reasonable feature addition:

writer.add_image('sample', 'sample.png')

The rationale for closing #21 was that this sort of encoding is best left to the user. I disagree based on the following:

  1. tensorboardX already uses a PIL.Image instance to encode a numpy array to PNG bytes

  2. If given a PIL.Image instance, tensorboardX can skip the step of generating one from the numpy array (this is shown in the example)

  3. If given a string value as the second argument to writer.add_image, tensorboardX can trivially call PIL.Image.open and use that image instance to encode the PNG bytes for the summary

This PR is valid as a stand-alone example, but I also hope it makes the case to support the following enhancements to writer.add_image:

  • Support a string arg for image, which is used in a call to PIL.Image.open
  • Support a PIL Image instance directly

I'm happy to implement those changes in another PR but I wanted to start this discussion with an example (useful in its own right).

@codecov-io
Copy link

codecov-io commented Aug 22, 2019

Codecov Report

Merging #490 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #490      +/-   ##
==========================================
+ Coverage   89.07%   89.12%   +0.05%     
==========================================
  Files          37       37              
  Lines        2562     2566       +4     
==========================================
+ Hits         2282     2287       +5     
+ Misses        280      279       -1
Impacted Files Coverage Δ
tensorboardX/embedding.py 100% <0%> (ø) ⬆️
tensorboardX/writer.py 75.17% <0%> (+0.08%) ⬆️
tensorboardX/pytorch_graph.py 91.78% <0%> (+0.68%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 366bc8f...18e2690. Read the comment docs.

@lanpa
Copy link
Owner

lanpa commented Oct 4, 2019

Sorry for missing this PR. It would be good to minimize the overhead to convert the data back and forth. As of your suggestion, I think passing "string arg for image" might conflict with the usage of caffe2 blob name (use string to index tensor). I would suggest using writer.add_image_from_file to separate the code section instead of adding logics inside the add_image function. This function will be much more easy to support other formats in the future.

def add_image_from_file(self, tag, filename_or_PILobject, global_step=None, walltime=None):
  # if("PIL onj"):
  # if("png"):
  # if("jpg"):

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants