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.

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

Add support for Transforms.Scale([w, h]) with specific width and height #133

Merged
merged 9 commits into from
Apr 6, 2017
Merged

Add support for Transforms.Scale([w, h]) with specific width and height #133

merged 9 commits into from
Apr 6, 2017

Conversation

yanghuanflc
Copy link
Contributor

if self.size is a int then scale the image with the shorter side,
otherwise if self.size is a list then scale the image to self.size
directly

@yanghuanflc
Fixed border missing on bottom and right side using make_grid
e489f16
@yanghuanflc
Merge remote-tracking branch 'pytorch/master'
f66d080
@yanghuanflc
Add support for Transforms.Scale([h, w]) with specific height and width
7e60249 
if self.size is a int then scale the image with the shorter side,
otherwise if self.size is a list then scale the image to self.size
directly
@yanghuanflc yanghuanflc mentioned this pull request Mar 28, 2017
Closed
@fmassa
Copy link
Member

fmassa commented Mar 29, 2017

Looks good! A few comments:

  • It might be good to assert in the constructor that size is either a number or a 2-element tuple.
  • Could you modify the inline documentation, as well as the documentation in README to reflect those changes?
  • The linter is complaining. You can check where it's complaining by running flake8 .

@yanghuanflc
Copy link
Contributor Author

Ok, I will refine the code and send a PR again.

@malreddysid
Copy link

malreddysid commented Mar 30, 2017 •
edited

Could you also add what I tried to do here? #136
I ran into some issue when I tried to create a pull request.

@yanghuanflc
Copy link
Contributor Author

I will try it today.

@malreddysid
Copy link

Thanks. I already tested the code in my PC. It's working fine.

@varunagrawal
Copy link
Contributor

@yanghuanflc @fmassa can you please get this PR merged. It is super useful and I wish to build on top of it to support numpy arrays as well.

@fmassa fmassa self-requested a review April 4, 2017 09:53
@fmassa
Copy link
Member

fmassa commented Apr 4, 2017

@yanghuanflc will you have the time to fix the comments above? If not, I'll look into completing it later today.

@yanghuanflc
Copy link
Contributor Author

Sorry for the delay, I'm busying with some events these days, I will fix it right now and send a PR again.

@yanghuanflc
Copy link
Contributor Author

Please review it again

@fmassa
Copy link
Member

fmassa commented Apr 4, 2017

This looks good. One last thing, could you please add another test case in here? After that it's good to be merged.
Thanks!

@fmassa
Copy link
Member

fmassa commented Apr 4, 2017

Actually, the linter is complaining again. Here are the messages:

./torchvision/transforms.py:118:1: W293 blank line contains whitespace
./torchvision/transforms.py:120:1: W293 blank line contains whitespace
./torchvision/transforms.py:121:74: W291 trailing whitespace
./torchvision/transforms.py:124:1: W293 blank line contains whitespace
./torchvision/transforms.py:135:38: E231 missing whitespace after ':'
./torchvision/transforms.py:135:38: E701 multiple statements on one line (colon)
./torchvision/transforms.py:135:39: E999 SyntaxError: invalid syntax
./torchvision/transforms.py:136:13: E113 unexpected indentation

@yanghuanflc
Fix linter problem
6309cae 
Fix linter problem
@yanghuanflc
Copy link
Contributor Author

Linter problem fixed, please review it again, sorry for all the inconvenience.

@yanghuanflc
Add test for Scale
aa092b2 
Add test for Scale
@yanghuanflc
Copy link
Contributor Author

Add test for Scale

@yanghuanflc
Add both tuple and list support for Scale.size
36fda0e 
Add both tuple and list support for Scale.size
@yanghuanflc
Copy link
Contributor Author

I also add 2-element list support for Scale.size with document modified.

fmassa
fmassa reviewed Apr 5, 2017
View reviewed changes
torchvision/transforms.py Outdated
@@ -124,7 +124,7 @@ class Scale(object):
"""

def __init__(self, size, interpolation=Image.BILINEAR):
assert isinstance(size, int) or (isinstance(size, tuple) and len(size) == 2)
assert isinstance(size, int) or ((isinstance(size, tuple) or isinstance(size, list)) and len(size) == 2)

This comment was marked as off-topic.

Sign in to view

fmassa
fmassa reviewed Apr 5, 2017
View reviewed changes
torchvision/transforms.py Outdated
@@ -115,29 +115,34 @@ def __call__(self, tensor):

class Scale(object):
"""Rescales the input PIL.Image to the given 'size'.
'size' will be the size of the smaller edge.
If 'size' is a 2-element tuple or list, it will be the exactly size to scale.

This comment was marked as off-topic.

Sign in to view

@fmassa
Copy link
Member

fmassa commented Apr 5, 2017

I added some last comments, after that it's good to go!

@yanghuanflc
Add order of Scale.size in document and test case for list type of Sc…
04935ed 
…ale.size

Add order of Scale.size in document and test case for list type of
Scale.size
@yanghuanflc
Copy link
Contributor Author

All comments fixed and add test case for both tuple and list type of Scale.size

@fmassa fmassa merged commit f6d49d8 into pytorch:master Apr 6, 2017
@fmassa
Copy link
Member

fmassa commented Apr 6, 2017

Thanks!

This was referenced Sep 3, 2017
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmassa fmassa fmassa left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@yanghuanflc @fmassa @malreddysid @varunagrawal