Skip to content

Conversation

@edgarriba
Copy link
Contributor

No description provided.

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.

Hey, sorry for the delay in reviewing.
This looks good for the most part, I made some small comments (mostly cosmetics). Could you address them?

if isinstance(pic, np.ndarray):
# handle numpy array
img = torch.from_numpy(pic.transpose((2, 0, 1)))
if len(pic.shape) >= 3:

This comment was marked as off-topic.

if len(pic.shape) >= 3:
img = torch.from_numpy(pic.transpose((2, 0, 1)))
else:
img = torch.from_numpy(pic.reshape((1,) + pic.shape))

This comment was marked as off-topic.

if torch.is_tensor(pic):
npimg = np.transpose(pic.numpy(), (1, 2, 0))
assert isinstance(npimg, np.ndarray), 'pic should be Tensor or ndarray'
if len(npimg.shape) < 3:

This comment was marked as off-topic.

npimg = np.transpose(pic.numpy(), (1, 2, 0))
assert isinstance(npimg, np.ndarray), 'pic should be Tensor or ndarray'
if len(npimg.shape) < 3:
npimg = np.reshape(npimg, npimg.shape + (1,))

This comment was marked as off-topic.

@alykhantejani
Copy link
Contributor

@edgarriba are you able to make the changes requested by @fmassa?
Also, it would be good to add another case to test_to_tensor to test for single channel images.

@edgarriba
Copy link
Contributor Author

@alykhantejani @fmassa sure, I'll allocate some time during this weekend

import torch
import torchvision.transforms as transforms
import unittest
from parameterized import parameterized

This comment was marked as off-topic.

This comment was marked as off-topic.


if accimage is not None and isinstance(pic, accimage.Image):
nppic = np.zeros([pic.channels, pic.height, pic.width], dtype=np.float32)
nppic=np.zeros([pic.channels, pic.height, pic.width], dtype=np.float32)

This comment was marked as off-topic.

import torch
import torchvision.transforms as transforms
import unittest
from parameterized import parameterized

This comment was marked as off-topic.

('3channel', 3, 4, 4),
('1channel', 1, 4, 4),
])
def test_pil_to_tensor(self, _, channels, height, width):

This comment was marked as off-topic.

@alykhantejani
Copy link
Contributor

@edgarriba Thanks for this - in general it looks good just a few small things to fix. I don't think we want to add another dependency to the library if we can avoid it. Would you be able to make this change and the linting ones?

@utkuozbulak
Copy link
Contributor

utkuozbulak commented Jun 7, 2018

Checks for image/numpy channels are implemented with refactoring of transforms to functional and transforms in #311

if not(_is_pil_image(pic) or _is_numpy_image(pic)):
raise TypeError('pic should be PIL Image or ndarray. Got {}'.format(type(pic)))

Although single channel images (HxW) still produce following error: ValueError: axes don't match array due to

if isinstance(pic, np.ndarray):
# handle numpy array
img = torch.from_numpy(pic.transpose((2, 0, 1)))

Wouldn't it be easier to just expand these arrays with a simple if, like:

...
    if isinstance(pic, np.ndarray):  ## Existing
        # handle numpy array
        if pic.ndim == 2:  ## Addition
            # expand by one dimension as the last channel  ## Addition
        img = torch.from_numpy(pic.transpose((2, 0, 1)))  ## Existing
...

Either way, this PR might as well be closed as a clean up due to massive conflicts?

@fmassa
Copy link
Member

fmassa commented Jun 7, 2018

I'm planning on modifying the underlying data structure that we use internally (instead of using a PIL image, have a custom class), so that those corner cases can be fixed.
I'll open an issue with the description of the proposal once I get the chance.

@utkuozbulak
Copy link
Contributor

Sounds like a plan, I would like to work on that if you will need a pair of hands.

rajveerb pushed a commit to rajveerb/vision that referenced this pull request Nov 30, 2023
* adding tag to handle evaluations by iterations

* better naming

* better naming round 2

* update the python package version
@edgarriba edgarriba closed this Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants