Transforms documentation clean-up #3200

voldemortX · 2020-12-22T08:34:03Z

A documentation clean-up that concerns transform.py functional.py functional_pil.py functional_tensor.py for issue #3071.
@datumbox After some initial checking, there are somethings I'm not certain:

Does the Sequence type represent only list and tuple in torchvision? I find that in Python other types (e.g. strings) can also count as a Sequence. Maybe we should clear it up in the docs, like maybe just say list or tuple.
Does the Posterize operation also only support uint8 images in PIL? I'm inclined to add "only support L or RGB" to all related transforms like I did with invert(). But maybe we can just let PIL throw the OSError.
Should we also add "This transform supports both PIL and Tensor, ..." in the documentation for AutoAugment as well?
Maybe we could delete the private function docs, as mentioned by @vfdev-5 here.

And there are some other things I found after cleaning the docs up, maybe they can be addressed in the future:

In Pad (or RandomCrop, it also uses padding), it seems tensors only support single value fill. Also I'm not sure whether str is really needed here by PIL, what is the use case here?
Some type checking is also Sequence, maybe we should check explicitly for tuple and list. Or is this the intended implementation?

vfdev-5

Thanks for the PR @voldemortX

Does the Sequence type represent only list and tuple in torchvision? I find that in Python other types (e.g. strings) can also count as a Sequence. Maybe we should clear it up in the docs, like maybe just say list or tuple.

Yes, sequence is also a string. I'd say here it is implicitly assumed that string input for, for example, size="abc" is completely wrong... also isinstance(size, Sequence) is a bit shorter then isinstance(size, (list, tuple)).

Maybe we could delete the private function docs

I'd say yes

torchvision/transforms/functional.py

datumbox · 2020-12-22T10:14:39Z

To add on the reply of @vfdev-5:

Does the Posterize operation also only support uint8 images in PIL? I'm inclined to add "only support L or RGB" to all related transforms like I did with invert(). But maybe we can just let PIL throw the OSError.

Yes it's only supported in PIL when the image is uint8. In this case it's important to clarify to the users that the tensor should be of data type uint8. Currently an image can be RGB but represented as floats. If that's the case, the transform won't work.

Should we also add "This transform supports both PIL and Tensor, ..." in the documentation for AutoAugment as well?

Not necessary because supporting a transform in both backends is our default. We only specific when something is not supported in any of the two backends.

voldemortX · 2020-12-22T10:18:14Z

Not necessary because supporting a transform in both backends is our default. We only specific when something is not supported in any of the two backends.

Maybe we should remove that support both backend statement for other docs as well?

voldemortX · 2020-12-22T10:24:16Z

Yes, sequence is also a string. I'd say here it is implicitly assumed that string input for, for example, size="abc" is completely wrong... also isinstance(size, Sequence) is a bit shorter then isinstance(size, (list, tuple)).

Does that need to be like unified in the doc to use sequence everywhere or int and tuple everywhere? @vfdev-5

vfdev-5 · 2020-12-22T10:28:47Z

Yes, sequence is also a string. I'd say here it is implicitly assumed that string input for, for example, size="abc" is completely wrong... also isinstance(size, Sequence) is a bit shorter then isinstance(size, (list, tuple)).

Does that need to be like unified in the doc to use sequence everywhere or int and tuple everywhere? @vfdev-5

Yes, in the docs it is nice to unify the type description. In the code this may be not possible due to torchscript...
I'd say most of the times, input type is "sequence or number"... as may be int or float or list or tuple...

datumbox

Thanks a lot for the PR. These are much needed changes that hopefully will make the documentation clearer. :)

I left a couple of comments. Let me know what you think.

torchvision/transforms/functional.py

datumbox · 2020-12-22T10:25:06Z

torchvision/transforms/functional_tensor.py

@@ -678,7 +678,7 @@ def pad(img: Tensor, padding: List[int], fill: int = 0, padding_mode: str = "con
            this is the padding for the left, top, right and bottom borders
            respectively. In torchscript mode padding as single int is not supported, use a tuple or
            list of length 1: ``[padding, ]``.
-        fill (int): Pixel fill value for constant fill. Default is 0.
+        fill (int, float): Pixel fill value for constant fill. Default is 0.


As discussed in comments, let's go ahead and remove all docs from functional_tensor.py and torchvision/transforms/functional_pil.py

torchvision/transforms/transforms.py

datumbox · 2020-12-22T10:35:15Z

torchvision/transforms/transforms.py

@@ -1707,6 +1713,7 @@ class RandomInvert(torch.nn.Module):
    The image can be a PIL Image or a torch Tensor, in which case it is expected
    to have [..., H, W] shape, where ... means an arbitrary number of leading
    dimensions.
+    For PIL images, only mode "L" and "RGB" is supported.


I think that's true for both PIL and Tensor.

Let me check if my understanding is correct: it should be that all methods that use _lut() supports only L and RGB for PIL, and 1/3 channel images in tensor?

Both require that they are L or RGB. See the entering invert() functional method:

vision/torchvision/transforms/functional.py

Lines 1177 to 1178 in 90645cc

def invert(img: Tensor) -> Tensor:

"""Invert the colors of an RGB/grayscale PIL Image or torch Tensor.

Should I specify the related docs (invert, solarize, etc.) like, for example:

"""Invert the colors of an RGB/grayscale image.

Args: img (PIL Image or Tensor): L or RGB image to have its colors inverted. If img is a Tensor, it is expected to be in [..., 1 or 3, H, W] format, where ... means it can have an arbitrary number of leading dimensions. Returns: PIL Image or Tensor: Color inverted image.

"""

I mean, tensors don't really have the mode attribute.

EDIT:
All concerned functions include adjust_brightness(), adjust_sharpness(), adjust_gamma(), invert(), posterize(), solarize(), autocontrast(), equalize(). I'm not sure should they all be applied on RGB or L?

datumbox · 2020-12-22T10:38:55Z

Maybe we should remove that support both backend statement for other docs as well?

Sounds great to me. I saw you already done this in many places.

Yes, in the docs it is nice to unify the type description. In the code this may be not possible due to torchscript..

I agree. Torchscript might give you headaches. Perhaps ensure that our doc is consistent everywhere using Sequences in this PR and follow up with the proposed changes on another one if TorchScript plays nice?

voldemortX · 2020-12-22T14:31:03Z

I made some further changes based on above reviews. Let me know if I forgot something or got something wrong!
@vfdev-5 @datumbox

Also TODOs in future PRs maybe:

Try check only for number and sequence in the code.
Try unify tensor and PIL Image in pad().

datumbox

@voldemortX Apologies if I was not very clear. It's not our intention to mark the specific files with underscores at this point.

A very bad side-effect of doing so is we lose the entire git-blame history, so such changes need to be justified. Please revert the name change and I'll check again your PR once it's done.

voldemortX · 2020-12-22T14:53:11Z

@voldemortX Apologies if I was not very clear. It's not our intention to mark the specific files with underscores at this point.

A very bad side-effect of doing so is we lose the entire git-blame history, so such changes need to be justified. Please revert the name change and I'll check again your PR once it's done.

Sorry... Should I revert all commits after 08fba9b?

datumbox · 2020-12-22T14:56:23Z

Sorry... Should I revert all commits after 08fba9b?

No worries mate. :) You can just undo the filename change in a new commit or revert the specific commit or however else you prefer. When we merge the PRs, the history is squashed so there will be no side-effects on master.

voldemortX · 2020-12-22T15:04:04Z

@datumbox Is this new commit ok?

datumbox

@voldemortX Awesome work! Thanks a lot for spending the time not just removing the duplicate docs, but finding out what is currently supported and what's not. This is a major improvement on our documentation.

voldemortX · 2020-12-23T08:50:32Z

After seeing this issue #3206 , we might need to also clarify the doc for erase(). I think it does support batch input (although the same randomness across the whole batch) right?

datumbox · 2020-12-23T09:16:03Z

Yes it does but the input needs to be a tensor, not a PIL image.

Summary: * Initial doc clean-up * Remove all private docs * Rename files * Highlight backend inconsistencies * Sequence and number * [Need checking] AutoAugment related doc change * Revert name changes Reviewed By: datumbox Differential Revision: D25954563 fbshipit-source-id: 3b73d924ec4e23d58416a8d38b554b4e16e64059

Initial doc clean-up

af5594f

facebook-github-bot added the cla signed label Dec 22, 2020

vfdev-5 reviewed Dec 22, 2020

View reviewed changes

torchvision/transforms/functional.py Outdated Show resolved Hide resolved

datumbox reviewed Dec 22, 2020

View reviewed changes

voldemortX added 5 commits December 22, 2020 19:27

Remove all private docs

7e6754a

Rename files

08f6a9b

Highlight backend inconsistencies

ec978bc

Sequence and number

aea159a

[Need checking] AutoAugment related doc change

6116b58

datumbox suggested changes Dec 22, 2020

View reviewed changes

Revert name changes

9b39bcb

datumbox approved these changes Dec 22, 2020

View reviewed changes

datumbox changed the title ~~[WIP] Transforms documentation clean-up~~ Transforms documentation clean-up Dec 22, 2020

Merge branch 'master' into issue3071

787b3dd

datumbox merged commit 7b9d30e into pytorch:master Dec 23, 2020

This was referenced Jan 26, 2021

Tensor pad transform limitations doc #3295

Merged

[WIP] Unify checking for Sequence and Number #3318

Closed

voldemortX mentioned this pull request Apr 21, 2021

Various doc fixes for transforms #3704

Merged

vfdev-5 mentioned this pull request Jun 28, 2022

Minor fix antialias arg #6209

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transforms documentation clean-up #3200

Transforms documentation clean-up #3200

voldemortX commented Dec 22, 2020 •

edited

Loading

vfdev-5 left a comment

datumbox commented Dec 22, 2020 •

edited

Loading

voldemortX commented Dec 22, 2020

voldemortX commented Dec 22, 2020

vfdev-5 commented Dec 22, 2020 •

edited

Loading

datumbox left a comment

datumbox Dec 22, 2020

datumbox Dec 22, 2020

voldemortX Dec 22, 2020 •

edited

Loading

datumbox Dec 22, 2020

voldemortX Dec 22, 2020 •

edited

Loading

datumbox commented Dec 22, 2020 •

edited

Loading

voldemortX commented Dec 22, 2020

datumbox left a comment

voldemortX commented Dec 22, 2020 •

edited

Loading

datumbox commented Dec 22, 2020

voldemortX commented Dec 22, 2020

datumbox left a comment

voldemortX commented Dec 23, 2020

datumbox commented Dec 23, 2020

	def invert(img: Tensor) -> Tensor:
	"""Invert the colors of an RGB/grayscale PIL Image or torch Tensor.

Transforms documentation clean-up #3200

Transforms documentation clean-up #3200

Conversation

voldemortX commented Dec 22, 2020 • edited Loading

vfdev-5 left a comment

Choose a reason for hiding this comment

datumbox commented Dec 22, 2020 • edited Loading

voldemortX commented Dec 22, 2020

voldemortX commented Dec 22, 2020

vfdev-5 commented Dec 22, 2020 • edited Loading

datumbox left a comment

Choose a reason for hiding this comment

datumbox Dec 22, 2020

Choose a reason for hiding this comment

datumbox Dec 22, 2020

Choose a reason for hiding this comment

voldemortX Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

datumbox Dec 22, 2020

Choose a reason for hiding this comment

voldemortX Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

datumbox commented Dec 22, 2020 • edited Loading

voldemortX commented Dec 22, 2020

datumbox left a comment

Choose a reason for hiding this comment

voldemortX commented Dec 22, 2020 • edited Loading

datumbox commented Dec 22, 2020

voldemortX commented Dec 22, 2020

datumbox left a comment

Choose a reason for hiding this comment

voldemortX commented Dec 23, 2020

datumbox commented Dec 23, 2020

voldemortX commented Dec 22, 2020 •

edited

Loading

datumbox commented Dec 22, 2020 •

edited

Loading

vfdev-5 commented Dec 22, 2020 •

edited

Loading

voldemortX Dec 22, 2020 •

edited

Loading

voldemortX Dec 22, 2020 •

edited

Loading

datumbox commented Dec 22, 2020 •

edited

Loading

voldemortX commented Dec 22, 2020 •

edited

Loading