Skip to content

Conversation

surgan12
Copy link
Contributor

@surgan12 surgan12 commented Sep 6, 2019

closes #1299

cc : @fmassa


def save_image(tensor, filename, nrow=8, padding=2,
normalize=False, range=None, scale_each=False, pad_value=0):
def save_image(tensor, fp, nrow=8, padding=2,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we add the format option, shouldn't we allow all additional parameters (dpi for .png, ...)for Image.save()? We could add a **kwargs or save_kwargs which is simply passed to im.save().

Copy link
Contributor Author

@surgan12 surgan12 Sep 6, 2019

Choose a reason for hiding this comment

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

Sure, they could be added based on the use cases.
IMO file option provides an interface to write as bytes and this might be used in a couple of cases .
What do you think ? @pmeier

Copy link
Collaborator

Choose a reason for hiding this comment

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

To be clear: I'm in favour of adding this as you did. I was just asking myself why we should only include the format option although Image.save() has a lot more options for the different file formats. I think if we give the user more control over the options we might as well give him all options. This of course does not need to happen in this PR. I just wanted to put it out there, so I or we don't forget it.

Copy link
Contributor Author

@surgan12 surgan12 Sep 6, 2019

Choose a reason for hiding this comment

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

@pmeier , I also feel the same, providing a full control over the save would be a better thing for the user.
Also the save_image in the current utils only differs in the make_grid part and there should be no harm in making the rest similar to PIL.

@fmassa fmassa closed this Sep 10, 2019
@fmassa fmassa reopened this Sep 10, 2019
Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I have a question: can't the user add a .name attribute to their file object, so that pillow knows which format to use? Would this work around the need of the format?

@pmeier
Copy link
Collaborator

pmeier commented Sep 10, 2019

Official documentation states:

format – Optional format override. If omitted, the format to use is determined from the filename extension. If a file object was used instead of a filename, this parameter should always be used.

If we go "by the book" we should include format parameter if we want to write to file objects.

That being said, I have no experience with this. Maybe @kracwarlock (author of #1299 ) can offer some insights, if this works for him.

@surgan12
Copy link
Contributor Author

surgan12 commented Sep 11, 2019

@fmassa , surely this is going to be a workaround.
The only thing that bothers me is that everytime user would have to choose a dummy name like (foo.png) which seems very inelegant to me.
However, we can add this workaround to docs if you want.

@surgan12
Copy link
Contributor Author

@fmassa any further comments ?

@fmassa
Copy link
Member

fmassa commented Sep 23, 2019

Sure, let's do it this way, although I think that I would like to avoid having to make the function signature more complicated.

@fmassa fmassa merged commit ef67fd9 into pytorch:master Sep 23, 2019
fmassa added a commit to fmassa/vision-1 that referenced this pull request Sep 24, 2019
fmassa added a commit that referenced this pull request Sep 24, 2019
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.

allow utils.save_image to write to a file object

3 participants