Skip to content

introduce nearest-exact interpolation#6754

Merged
pmeier merged 5 commits intopytorch:mainfrom
pmeier:exact-nearest
Oct 13, 2022
Merged

introduce nearest-exact interpolation#6754
pmeier merged 5 commits intopytorch:mainfrom
pmeier:exact-nearest

Conversation

@pmeier
Copy link
Copy Markdown
Contributor

@pmeier pmeier commented Oct 12, 2022

Addresses the first part of #6645. As discussed offline with @datumbox, exposing this mode is non-controversial. Whether or not we should make it the default and how the transition period should look like is another matter entirely. Let's do that if more pressing matters are finished.

@vfdev I would like to have your input on how to test this properly on the stable transforms test.

@pmeier pmeier requested review from datumbox and vfdev-5 October 12, 2022 12:41
pmeier
pmeier commented Oct 12, 2022
View reviewed changes
torchvision/transforms/functional.py
InterpolationMode.NEAREST: 0,
InterpolationMode.BILINEAR: 2,
InterpolationMode.BICUBIC: 3,
InterpolationMode.NEAREST_EXACT: 0,
Copy link
Copy Markdown
Contributor Author

@pmeier pmeier Oct 12, 2022

Choose a reason for hiding this comment

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

The only instance of where we want the reverse mapping, i.e. from int to InterpolationMode, comes from the _interpolation_modes_from_int function above. AFAIK, this is only needed until the deprecation period for passing int's is over and will be removed afterwards. Is that true?

If yes, I think it is ok to also assign 0 as the Pillow interpolation mode here as we already do for InterpolationMode.NEAREST. For the Pillow side this makes sense, since they only have one nearest interpolation.

The only downside to this is that users cannot access InterpolationMode.NEAREST_EXACT through and int. But since that is deprecated anyway, I don't think this is an issue.

Copy link
Copy Markdown
Contributor

@datumbox datumbox Oct 12, 2022

Choose a reason for hiding this comment

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

I think as long as 0 is assigned back to InterpolationMode.NEAREST for BC, we should be OK.

@pmeier
Copy link
Copy Markdown
Contributor Author

pmeier commented Oct 12, 2022 •
edited
Loading

A related and interesting question is: do we want to use InterpolationMode.EXACT_NEAREST for our resize_mask kernel?

Right now, we ignore the input

vision/torchvision/prototype/features/_mask.py

Lines 47 to 55 in 7d36d26

def resize( # type: ignore[override]
self,
size: List[int],
interpolation: InterpolationMode = InterpolationMode.NEAREST,
max_size: Optional[int] = None,
antialias: bool = False,
) -> Mask:
output = self._F.resize_mask(self, size, max_size=max_size)
return Mask.wrap_like(self, output)

and simply hardcode InterpolationMode.NEAREST:

vision/torchvision/prototype/transforms/functional/_geometry.py

Line 156 in 7d36d26

output = resize_image_tensor(mask, size=size, interpolation=InterpolationMode.NEAREST, max_size=max_size)

Since this kernel did not exist before, there are no BC concerns here for using InterpolationMode.EXACT_NEAREST.

datumbox
datumbox approved these changes Oct 12, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

LGTM!

@vfdev-5 any concerns on your side?

torchvision/transforms/functional.py
InterpolationMode.NEAREST: 0,
InterpolationMode.BILINEAR: 2,
InterpolationMode.BICUBIC: 3,
InterpolationMode.NEAREST_EXACT: 0,
Copy link
Copy Markdown
Contributor

@datumbox datumbox Oct 12, 2022

Choose a reason for hiding this comment

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

I think as long as 0 is assigned back to InterpolationMode.NEAREST for BC, we should be OK.

@vfdev-5
Copy link
Copy Markdown
Contributor

vfdev-5 commented Oct 12, 2022

All good to me as well

NicolasHug
NicolasHug reviewed Oct 12, 2022
View reviewed changes
Copy link
Copy Markdown
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 but should we add some tests in the non-prototype area to make sure these work as expected?

(On a side note, we recently made this mode at least 2X faster on channels_last images pytorch/pytorch#86361)

@pmeier
Copy link
Copy Markdown
Contributor Author

pmeier commented Oct 12, 2022

LGTM but should we add some tests in the non-prototype area to make sure these work as expected?

Yes, we should. #6754 (comment)

@vfdev I would like to have your input on how to test this properly on the stable transforms test.

(On a side note, we recently made this mode at least 2X faster on channels_last images pytorch/pytorch#86361)

2x faster compared to "nearest" or compared to the previous implementation of "nearest-exact"? If it is the former, could that be a possible perf optimization we could do on the prototype references to squeeze out more?

@NicolasHug
Copy link
Copy Markdown
Member

NicolasHug commented Oct 12, 2022

compared to the previous implementation of "nearest-exact"?

This one. There's nothing to do on torchvision's side to benefit from it, it'll come from core

@vfdev-5
Copy link
Copy Markdown
Contributor

vfdev-5 commented Oct 12, 2022

nearest and nearest-exact are doing almost the same computations except the way how weights were computed ... I'd expect the same perfs between them

@pmeier pmeier merged commit bdc5556 into pytorch:main Oct 13, 2022
@pmeier pmeier deleted the exact-nearest branch October 13, 2022 11:59
facebook-github-bot pushed a commit that referenced this pull request Oct 17, 2022
@facebook-github-bot
[fbsync] introduce nearest-exact interpolation (#6754)
71b3c4a 
Summary:
* introduce nearest-exact interpolation

* update prototype tests

* update stable tests

Reviewed By: NicolasHug

Differential Revision: D40427462

fbshipit-source-id: bc3064a7167c03e9d6bad21d523a3132e853b80f
@NicolasHug NicolasHug mentioned this pull request May 14, 2024
Open
@tomaarsen tomaarsen mentioned this pull request Aug 5, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NicolasHug NicolasHug NicolasHug left review comments

@vfdev-5 vfdev-5 Awaiting requested review from vfdev-5

+1 more reviewer

@datumbox datumbox datumbox approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla signed enhancement module: transforms

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pmeier @vfdev-5 @NicolasHug @datumbox @facebook-github-bot