Skip to content

Conversation

@NicolasHug
Copy link
Member

@NicolasHug NicolasHug commented Aug 21, 2023

  • Reorganize API ref in to 2 separate sections: one for V2, one for V1
  • Document the v2 functional (docstrings still to be done)
  • Lots of re-writes and additional info
  • Make v2 the "officially recommended" namespace

Will follow-up with changes to the gallery examples when this is done.

cc @vfdev-5

@NicolasHug NicolasHug requested review from pmeier and vfdev-5 August 21, 2023 11:20
@pytorch-bot
Copy link

pytorch-bot bot commented Aug 21, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/7859

Note: Links to docs will display an error until the docs builds have been completed.

❌ 6 New Failures, 1 Unrelated Failure

As of commit 7f6a39d with merge base 2c44eba (image):

NEW FAILURES - The following jobs have failed:

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

This comment was automatically generated by Dr. CI and updates every 15 minutes.

Comment on lines +282 to +284
# Inplace operations on datapoints like ``obj.add_()`` will preserve the type of
# ``obj``. However, the **returned** value of inplace operations will be a pure
# tensor:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more of a drive-by. Turns out those inplace operations aren't really exceptions at all.


.. currentmodule:: torchvision.transforms

Torchvision supports common computer vision transformations in the
Copy link
Member Author

@NicolasHug NicolasHug Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the text below is mostly new. There was very little copy/pasting. Mostly re-writing from scratch. Best to ignore the diff when reviewing.

v2.RandomResize
RandomCrop

Functionals
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted to separate the functionals into [subsubsubsub]sections like that. The alternative is to put e.g. resize just below Resize, but I found that to hurt readability of the rendered docs because it creates tables that are too long, and it becomes hard to figure out which transforms are actually supported.

(I didn't change the way we document the v1 stuff)

@vfdev-5
Copy link
Contributor

vfdev-5 commented Aug 21, 2023

@NicolasHug great work rewriting transforms.rst !

Copy link
Contributor

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks a lot !

Copy link
Contributor

@pmeier pmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments inline. Otherwise, I stumbled over this:

class ToPILImage(Transform):
"""[BETA] Convert a tensor or an ndarray to PIL Image - this does not scale values.

This does not come from this PR, but might be worth fixing here.

For the most common case, i.e. tensor input and omitting the mode parameter, we do in fact scale the values:

if isinstance(pic, torch.Tensor):
if pic.is_floating_point() and mode != "F":
pic = pic.mul(255).byte()

Meaning, this transform will fail for users that do ToPILImage(my_float_image_tensor * 255)

Anyway, that is not really important. Thanks Nicolas for the major rewrite!

Comment on lines +155 to +157
Transforms tend to be sensitive to the input strides / memory layout. Some
transforms will be faster with channels-first images while others prefer
channels-last. You may want to experiment a bit if you're chasing the very
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we had user confusion before, should we point out here that we are talking about the memory layout and not the actual shape of the tensor?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in 7f6a39d

Comment on lines +189 to +190
parametrization. The ``get_params()`` class method of the transforms class
can be used to perform parameter sampling when using the functional APIs.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A public get_params is only available for BC for v1 transforms. Should we advertise this here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is still the only way to sample parameters, right? I don't think we just added it for BC, we added it for feature completeness.

@NicolasHug NicolasHug merged commit 37081ee into pytorch:main Aug 22, 2023
facebook-github-bot pushed a commit that referenced this pull request Sep 6, 2023
Summary: (Note: this ignores all push blocking failures!)

Reviewed By: matteobettini

Differential Revision: D48900378

fbshipit-source-id: 2746d77f5c3a01223a98a20ba12f217b89d9652b

Co-authored-by: Philip Meier <[email protected]>
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.

4 participants