Skip to content
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

allow len 1 sequences for fill with PIL #7928

Merged
merged 5 commits into from Sep 4, 2023
Merged

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Sep 1, 2023

The length 1 sequences for fill were just a workaround back when JIT didn't support Union. Since PIL is not restricted by JIT, we never added support for it.

This means however, that for example F.rotate(img_tensor, fill=[127]) works, while F.rotate(img_pil, fill=[127]) doesn't.

That makes parametrized tests harder, e.g.

  • xfails_pil_if_fill_sequence_needs_broadcast = xfails_pil(
    "PIL kernel doesn't support sequences of length 1 for `fill` if the number of color channels is larger.",
    condition=fill_sequence_needs_broadcast,
    )
  • if isinstance(input, PIL.Image.Image) and isinstance(value, (tuple, list)) and len(value) == 1:
    pytest.xfail("F._pad_image_pil does not support sequences of length 1 for fill.")

This PR adds support for length 1 sequences of fill for the PIL backend. Note, that I've implemented the fix in v1, since all our v2 PIL ops with a fill parameter are thin wrappers around the v1 kernel. LMK if that is ok. Otherwise we can also do it in a more verbose way in v2.

cc @vfdev-5

@pytorch-bot
Copy link

pytorch-bot bot commented Sep 1, 2023

@pmeier pmeier marked this pull request as ready for review September 1, 2023 12:49
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

LGTM if green, thanks. Let's just make sure we don't need to update any docstring before merging

@pmeier
Copy link
Collaborator Author

pmeier commented Sep 1, 2023

Docstrings for v1 look this:

fill (sequence or number): Pixel fill value for the area outside the transformed
image. Default is ``0``. If given a number, the value is used for all bands respectively.

fill (sequence or number, optional): Pixel fill value for the area outside the transformed
image. If given a number, the value is used for all bands respectively.
.. note::
In torchscript mode single int/float value is not supported, please use a sequence
of length 1: ``[value, ]``.

  • Only the functional mentions the sequence of length 1 format at all
  • Neither the transform nor the functional mention that one can use it for eager as well

IMO, we don't need to update the docs here. The fact that PIL also supports length 1 sequences is more a QoL thing. I don't expect users to actively use it. But if they do, it is nice that the PIL and Tensor backend have the same behavior.

@pmeier pmeier merged commit d0e16b7 into pytorch:main Sep 4, 2023
32 of 49 checks passed
@pmeier pmeier deleted the pil-fill branch September 4, 2023 07:34
pmeier added a commit to pmeier/vision that referenced this pull request Sep 8, 2023
facebook-github-bot pushed a commit that referenced this pull request Sep 26, 2023
Reviewed By: matteobettini

Differential Revision: D49600777

fbshipit-source-id: 1b8731b6ef2e42feba0bde4dc7ee23bec6bfbe46
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.

None yet

3 participants