-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Fix for #409 #673
Fix for #409 #673
Conversation
@alykhantejani can you please review this. |
There was a problem hiding this 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 few questions and comments.
torchvision/transforms/functional.py
Outdated
@@ -105,9 +105,17 @@ def to_pil_image(pic, mode=None): | |||
Returns: | |||
PIL Image: Image converted to PIL Image. | |||
""" | |||
if not(_is_numpy_image(pic) or _is_tensor_image(pic)): | |||
if not(torch.is_tensor(pic) or isinstance(pic, np.ndarray)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use instead isisntance(pic, torch.Tensor)
? This is the recommended way now to checking if something is an instance of a Tensor or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
torchvision/transforms/functional.py
Outdated
raise TypeError('pic should be Tensor or ndarray. Got {}.'.format(type(pic))) | ||
|
||
elif torch.is_tensor(pic): | ||
if pic.ndimension() != 3: | ||
raise ValueError('pic should be 3 dimensional. Got {} dimensions.'.format(pic.ndimension())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remind me again why the code works for 2d np arrays, but not 2d tensors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The is_numpy_image
function checks for both 2D and 3D arrays. I assumed this was for some other functionality and left that in to avoid breaking something else. I can add code to work for 2D tensors as well if you'd like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. But there was a bug before for 2d arrays, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. The bug has been for 2D tensors - we get a TypeError
exception. There was functionality for 2D arrays before this PR.
…refactored the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Fix for #409