Skip to content

save_image behavior #24

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

Closed
rtqichen opened this issue Jan 7, 2017 · 4 comments · Fixed by #99
Closed

save_image behavior #24

rtqichen opened this issue Jan 7, 2017 · 4 comments · Fixed by #99

Comments

@rtqichen
Copy link
Contributor

rtqichen commented Jan 7, 2017

https://github.com/pytorch/vision/blob/master/torchvision/utils.py#L52

The .mul(0.5).add(0.5) in this line seems to be assuming that the input values are in [-1,1]? Isn't the Torch convention to use [0,1] for images?

@soumith
Copy link
Member

soumith commented Jan 7, 2017

you're right. i first did that as a hack and transferred it as-is into vision.
I'll do this:

  1. Introduce kwargs called: normalize, range, which if normalize=True -- would directly re-range the input into [0, 255] by per-instance renormalization, and then save it.
  2. If normalize=True and range=(min, max), it will use the min, max values to renormalize before saving.
  3. if normalize=False, then will save the input image as-is, i.e. with the exact current range.

@rtqichen
Copy link
Contributor Author

rtqichen commented Jan 7, 2017

What about something similar to the Lua Torch's image.display() behavior:

  • range=None (default) uses the max and min of the input images to renormalize into [0,255].
  • range=(min,max) (specified by the user) uses the provided min/max values to clamp then renormalize.

(The first option can be per-instance, but I think using the min/max across all provided images makes more sense.)

The difference here is that by default it's better to be able to handle all ranges instead of assuming a certain range. (I think normalize=False is the intended default value? Otherwise, using normalize=False and range=(0,255) is equivalent anyway.)

@stepelu
Copy link

stepelu commented Mar 2, 2017

Personally, I vote for having a default behavior that assumes a range [0, 1] and doesn't normalize the image.
Doing otherwise would result in issues like the one mentioned in pull 41 and potentially introduces bugs difficult to spot (as the normalization is "silent").

@soumith
Copy link
Member

soumith commented Mar 11, 2017

This is fixed in master by removing the offset, but definitely having a range parameter would be quite useful!

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 a pull request may close this issue.

3 participants