Replace get_image_size/num_channels with get_dimensions

[datumbox]

We would like to switch to an image_size convention that stores dimensions in (h, w) format instead of (w, h). Unfortunately the old get_image_size() method stored using the latter format due to PIL. Just changing the order in the new API while maintaining the same method name is a bad choice as it will certainly lead to confusion. An alternative approach is to introduce new "low-level" get_dimensions() kernels and a get_image_dimensions() utility method that return all image dimensions in the format (c, h, w).

[facebook-github-bot]

💊 CI failures summary and remediations

As of commit f5ff12b (more details on the Dr. CI page):

---

💚 💚 Looks good so far! There are no failures yet. 💚 💚

---

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

[datumbox]

Added a few comments to assist review:

[pmeier]

In general I agree with the direction of this. One caveat though: what if we have a transformation that needs only the height and width and we only have a BoundingBox or the like to extract it from? In such a case we have no access to the number of channels while the the image size is just height, width = bounding_box.image_size. This was my reason to keep the functions separate, since get_image_size accepts a wider range of inputs, while get_image_num_channels only applies to images.

[datumbox]

@pmeier Is this a problem? If the low-level kernel asks for only the information it needs aka image_size which is (h, w), then this probably shouldn't be an issue. Depending on what info the user has available, they can use the right method (the proposed for images or the attribute for bboxes etc). This is currently the situation for all kernels and this proposal doesn't modify this. Am I missing something?

[pmeier]

IIUC your proposal correctly, get_image_size and get_image_num_channels will be deprecated and later removed in favor of get_image_dimensions, correct? How would the implementation of a transformation look like that gets a sample dictionary that contains a bounding box and I need the image size from it?

[datumbox]

You will continue doing .image_size from the bbox, exactly to what you do now. See:

vision/torchvision/prototype/transforms/_geometry.py

Lines 47 to 49 in 04a3eab

	elif isinstance(input, features.BoundingBox):
	output = F.resize_bounding_box(input, self.size, image_size=input.image_size)
	return features.BoundingBox.new_like(input, output, image_size=self.size)

[pmeier]

That is only possible at the "dispatch" state, i.e. if we already look at a single item of the sample dictionary. However, sometimes we need the image_size at an earlier stage. For example,

vision/torchvision/prototype/transforms/_geometry.py

Lines 110 to 112 in f40c8df

	def _get_params(self, sample: Any) -> Dict[str, Any]:
	image = query_image(sample)
	width, height = F.get_image_size(image)

We currently do not support bounding boxes in the crop transforms, but I don't think it is a stretch to do so in the future. If we do, something like RandomResizedCrop should also work in cases we don't pass an image and just a BoundingBox. But get_image_dimensions cannot be applied to bounding boxes, since they don't carry any channels information. get_image_size however can be used (if we add support for bounding boxes there).

[datumbox]

something like RandomResizedCrop should also work in cases we don't pass an image and just a BoundingBox

I am not aware of any real world use-case where you Randomly Crop/resize a BBox without the Image. Doing so will destroy the parity between the image and the bbox and render the info useless.

[pmeier]

I am not aware of any real world use-case where you Randomly Crop/resize a BBox without the Image.

To quote yourself

Researchers do crazy things

Given that this doesn't solve anything right now, I would postpone this PR until we have a clearer picture. Or did I miss anything for which we need this change?

[pmeier]

Stamping to unblock.

[NicolasHug]

Changes to the main area LGTM.

Unfortunately the old get_image_size() method stored using the latter format due to PIL. Just changing the order in the new API while maintaining the same method name is a bad choice as it will certainly lead to confusion

Fully agree with this. Pushing this further, would it make sense to not have image_size on the prototype area, and just have image_dimensions that always returns C, H, W (with C potentially being None if it's unknown)? BBoxes could just do _, h, w = bbox.image_dimension ?

[datumbox]

@NicolasHug I think that's a perfectly viable proposal which will align fully the use of CHW everywhere across TorchVision. The only reason I see why we might not want to include the channels of the original image in BBox and Masks is because the low-level kernels don't need this info when they make operations on them. For example when you resize a bbox, you need the old and new width/height not the channels. But again, I personally value more consistency so I don't think that's a major deal breaker.

Another option would be to store height and width separately in the meta-data and stop passing image_size to methods. This will eliminate any confusion for what's what, though it will make things more verbose and will cause BC-breakages on APIs like resize().

@pmeier I'm positive that the use-case you describe is not something we need to support, as it's not a valid ML scenario. Concerning why we should move this forward, this PR affects your proposal at #5492. Note that instead of having multiple PRs with multiple proposals (something that is very hard to follow and share with the team for feedback), we should do iterations on top of the prototype area on main branch.

[gastruc]

Hi, running this tutorial https://pytorch.org/tutorials/intermediate/torchvision_tutorial.html, I get this error that may be linked with this merge:

File "/home/studio-lab-user/.conda/envs/default/lib/python3.9/site-packages/torch/utils/data/_utils/worker.py", line 287, in _worker_loop
    data = fetcher.fetch(index)
  File "/home/studio-lab-user/.conda/envs/default/lib/python3.9/site-packages/torch/utils/data/_utils/fetch.py", line 49, in fetch
    data = [self.dataset[idx] for idx in possibly_batched_index]
  File "/home/studio-lab-user/.conda/envs/default/lib/python3.9/site-packages/torch/utils/data/_utils/fetch.py", line 49, in <listcomp>
    data = [self.dataset[idx] for idx in possibly_batched_index]
  File "/tmp/ipykernel_46/1346308532.py", line 47, in __getitem__
    img, target = self.transforms(img, target)
  File "/home/studio-lab-user/sagemaker-studiolab-notebooks/transforms.py", line 26, in __call__
    image, target = t(image, target)
  File "/home/studio-lab-user/.conda/envs/default/lib/python3.9/site-packages/torch/nn/modules/module.py", line 1102, in _call_impl
    return forward_call(*input, **kwargs)
  File "/home/studio-lab-user/sagemaker-studiolab-notebooks/transforms.py", line 37, in forward
    _, _, width = F.get_dimensions(image)
AttributeError: module 'torchvision.transforms.functional' has no attribute  'get_dimensions'
 

How to fix this?

[datumbox]

@gastruc Please open an issue where you provide a minimal example that reproduces the problem.

From what I understand you are using get_dimensions() in your custom transforms.py file but you are installing a TorchVision version that doesn't support it. This PR was merged 22 hours ago, so it's likely that the change might not be available yet even on the nightly.

[prabhat00155]

This PR has introduced test_transforms.py::TestAccImage::test_accimage_crop test failure, which was not caught by CI as accimage is not installed in CI.