[BC-breaking] Introduced InterpolationModes and deprecated arguments: resample and fillcolor #2952

vfdev-5 · 2020-11-03T08:48:22Z

Related to #2479

Introduced InterpolationModes
Deprecated arguments: resample and fillcolor
Replaced by interpolation and fill

Context:

fill is mainly used: 4 times in transforms and 3 times in functional vs fillcolor is used 2 times in transforms and functional (RandomAffine and F.affine) => we can decide to replace fillcolor by fill and set a warning about argument deprecation.
interpolation is used 6 times in transforms/functional vs resample is used 4 times in transforms/functional => we can similarly decide to replace resample by interpolation and set a warning about argument deprecation.

Replaced by interpolation and fill

codecov · 2020-11-03T09:23:59Z

Codecov Report

Merging #2952 (453722c) into master (74de51d) will increase coverage by 1.30%.
The diff coverage is 91.83%.

@@            Coverage Diff             @@
##           master    #2952      +/-   ##
==========================================
+ Coverage   72.18%   73.49%   +1.30%     
==========================================
  Files          99       99              
  Lines        8806     8832      +26     
  Branches     1383     1397      +14     
==========================================
+ Hits         6357     6491     +134     
+ Misses       2012     1916      -96     
+ Partials      437      425      -12

Impacted Files	Coverage Δ
torchvision/transforms/functional_tensor.py	`74.14% <75.00%> (+0.47%)`	⬆️
torchvision/transforms/transforms.py	`80.83% <91.30%> (-1.75%)`	⬇️
torchvision/transforms/functional.py	`80.92% <100.00%> (+0.47%)`	⬆️
torchvision/transforms/functional_pil.py	`68.07% <100.00%> (ø)`
torchvision/ops/feature_pyramid_network.py	`91.20% <0.00%> (-3.30%)`	⬇️
torchvision/models/detection/anchor_utils.py	`92.10% <0.00%> (-2.70%)`	⬇️
torchvision/ops/deform_conv.py	`70.96% <0.00%> (-1.34%)`	⬇️
torchvision/models/detection/backbone_utils.py	`94.28% <0.00%> (-1.27%)`	⬇️
torchvision/models/densenet.py	`82.81% <0.00%> (-0.77%)`	⬇️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74de51d...ed1b509. Read the comment docs.

datumbox

@vfdev-5 Thanks for the PR.

I added one suggestion to improve BC during the deprecation period and marked all the places where this could happen. Nevertheless, if our policy is typically to only throw warnings feel free to ignore.

Also I noticed that in some methods you throw warnings while in others you directly replace the names. I assume that this is because the first are considered public API while the latter not. If that's true, then all good.

torchvision/transforms/functional.py

torchvision/transforms/transforms.py

vfdev-5

Thanks for the review @datumbox

I added one suggestion to improve BC during the deprecation period and marked all the places where this could happen. Nevertheless, if our policy is typically to only throw warnings feel free to ignore.

I agree that it is a good practice to provide until when the argument will remain in the API. I'd say by default, it can be 2 future minor releases, until 0.10.0. @fmassa any thoughts?

torchvision/transforms/functional.py

torchvision/transforms/transforms.py

datumbox

Thanks for the updates @vfdev-5, your last changes look great.

@fmassa Are you OK to merge?

fmassa

Thanks for the PR @vfdev-5 !

I have one comment regarding the warning message, but it is not blocking for merge.

On a separate note that I would like to discuss here:

When was planning for the Tensor support for transforms, I started to wonder if we should move away from the interpolate=0 enums from PIL (= Image.NEAREST etc), and instead use a more descriptive argument which could work even without the knowledge of PIL.

Two options came to my mind:

replace interpolate : int with mode : str as we have in torch.nn.functional.interpolate
provide our own Enum for the interpolation mode in torchvision (and hope it works on torchscript)

Thoughts?

fmassa · 2020-11-05T10:00:09Z

torchvision/transforms/functional.py


    Returns:
        PIL Image or Tensor: Transformed image.
    """
+    if resample is not None:
+        warnings.warn(
+            "Argument resample is deprecated and will be removed since v0.10.0. Please, use interpolation instead"


nit: I'm not a native English speaker, but I feel like since might not be the best word here. I would have said instead

Suggested change

"Argument resample is deprecated and will be removed since v0.10.0. Please, use interpolation instead"

"Argument resample is deprecated and will be removed in v0.10.0. Please, use interpolation instead"

This is a minor nit (which applies to the other messages as well), so I'm not considering this as a merge-blocking issue.

vfdev-5 · 2020-11-05T10:36:45Z

If we would like to use an enum for interpolate, there can be two potential problems with torchscript:

import torch
from enum import Enum

class Modes:
    NEAREST = 0
    BILINEAR = 1

def func(x: torch.Tensor, mode: Modes) -> torch.Tensor:
    if mode == Modes.NEAREST:
        return x + 1
    elif mode == Modes.BILINEAR:
        return x + 2
    else:
        return x

    
y = func(torch.rand(3, 4), Modes.NEAREST)
s_func = torch.jit.script(func)

if this code is launched inside jupyter gives an error:

/opt/conda/lib/python3.7/site-packages/torch/_utils_internal.py in get_source_lines_and_file(obj, error_msg)
     46     filename = None  # in case getsourcefile throws
     47     try:
---> 48         filename = inspect.getsourcefile(obj)
     49         sourcelines, file_lineno = inspect.getsourcelines(obj)
     50     except OSError as e:

/opt/conda/lib/python3.7/inspect.py in getsourcefile(object)
    682     Return None if no way can be identified to get the source.
    683     """
--> 684     filename = getfile(object)
    685     all_bytecode_suffixes = importlib.machinery.DEBUG_BYTECODE_SUFFIXES[:]
    686     all_bytecode_suffixes += importlib.machinery.OPTIMIZED_BYTECODE_SUFFIXES[:]

/opt/conda/lib/python3.7/inspect.py in getfile(object)
    651             if getattr(module, '__file__', None):
    652                 return module.__file__
--> 653         raise TypeError('{!r} is a built-in class'.format(object))
    654     if ismethod(object):
    655         object = object.__func__

TypeError: <class '__main__.Modes'> is a built-in class

if this code is launched as a script with if __name__ == "__main__": close, it gives another error:

Traceback (most recent call last):
  File "torchscript_enum.py", line 22, in <module>
    s_func = torch.jit.script(func)
  File "/opt/conda/lib/python3.7/site-packages/torch/jit/_script.py", line 940, in script
    qualified_name, ast, _rcb, get_default_args(obj)
RuntimeError: 
python value of type 'str' cannot be used as a value. Perhaps it is a closed over global variable? If so, please consider passing it in as an argument or use a local varible instead.:
  File "torchscript_enum.py", line 11
def func(x: torch.Tensor, mode: Modes) -> torch.Tensor:
    if mode == Modes.NEAREST:
               ~~~~~~~~~~~~~ <--- HERE
        return x + 1
    elif mode == Modes.BILINEAR:

vfdev-5 · 2020-11-05T10:47:48Z

However, the following works inside the script file

import torch
from enum import Enum

class Modes(Enum):
    NEAREST = 0
    BILINEAR = 1


def func(x: torch.Tensor, mode: Modes) -> torch.Tensor:
    if mode == Modes.NEAREST:
        return x + 1
    elif mode == Modes.BILINEAR:
        return x + 2
    else:
        return x

if __name__ == "__main__":
    y = func(torch.rand(3, 4), Modes.NEAREST)
    s_func = torch.jit.script(func)
    y = s_func(torch.rand(3, 4), Modes.NEAREST)

According to pytorch/pytorch#42963

EDIT: removed global Modes

fmassa · 2020-11-05T14:42:45Z

It's great that we can use Enum's with torchscript, but it's weird that we would need to have it set as a global beforehand.
cc @gmagogsfm is this expected?

vfdev-5 · 2020-11-05T16:41:58Z

Sorry, seems like global keyword can be omitted if used inside a script file.

…cate-resample-fillcolor

…nsight/vision into vfdev-5/deprecate-resample-fillcolor

torchvision/transforms/functional_pil.py

torchvision/transforms/functional_tensor.py

torchvision/transforms/functional.py

datumbox

The PR looks good. Thanks a lot for the changes, they look great.

I understand that it's a design decision to maintain BC on the high-level transforms.py but not with the lower-level functional implementations, correct? Also, I noticed that on the backend implementations you use raw values for default (ints or strings) instead of their enums. Is this because of a JIT limitation?

vfdev-5 · 2020-11-17T10:05:21Z

I understand that it's a design decision to maintain BC on the high-level transforms.py but not with the lower-level functional implementations, correct?

BC change is "required" to switch from PIL.Image resampling mode. Almost no change in lower-level functional implementations is a sort of minimal change design.

Also, I noticed that on the backend implementations you use raw values for default (ints or strings) instead of their enums. Is this because of a JIT limitation?

We can certainly find a way to reuse InterpolationModes inside private modules. As I said this is currently blocked by cyclic imports due to where InterpolationModes is defined. Remap from InterpolationModes to PIL modes happens inside F for each method. Remap from InterpolationModes to string values for torch functions is done with InterpolationModes.value attribute.

fmassa

Correct me if I'm wrong, but I think the BC-breaking change that is introduced in here will break all user code that uses the transforms and I if that's the case I'm not sure this is something we can do in torchvision.

fmassa · 2020-11-18T11:59:43Z

torchvision/transforms/functional.py

    if not isinstance(img, torch.Tensor):
-        return F_pil.resize(img, size=size, interpolation=interpolation)
+        pil_interpolation = pil_modes_mapping[interpolation]


Interesting that accessing this as a global works with torchscript. I suppose torchscript can compile this branch out and then we don't have an error, but I would expect this to fail if that wasn't the case

torchvision/transforms/functional.py

…nsight/vision into vfdev-5/deprecate-resample-fillcolor

fmassa

Thanks a lot!

I'm merging the PR now to unblock, but I have one last comment that I would like us to check: should it be InterpolationMode or InterpolationModes?

cc @vfdev-5 @datumbox for thoughts

fmassa · 2020-11-27T14:18:52Z

torchvision/transforms/functional.py


    Returns:
        PIL Image or Tensor: Rotated image.

    .. _filters: https://pillow.readthedocs.io/en/latest/handbook/concepts.html#filters

    """
+    if resample is not None:
+        warnings.warn(
+            "Argument resample is deprecated and will be removed since v0.10.0. Please, use interpolation instead"


Suggested change

"Argument resample is deprecated and will be removed since v0.10.0. Please, use interpolation instead"

"Argument resample is deprecated and will be removed in v0.10.0. Please, use interpolation instead"

datumbox · 2020-11-27T14:32:54Z

@fmassa I think enums are typically singular. Aka Color.Blue not Colors.Blue.

On a similar enum, I used ImageReadMode.

vfdev-5 · 2020-11-27T14:52:01Z

@fmassa @datumbox thanks for pointing out that. I agree that I can rename InterpolationModes to InterpolationMode.

… resample and fillcolor (pytorch#2952) * Deprecated arguments: resample and fillcolor Replaced by interpolation and fill * Updates according to the review * Added tests to check warnings and asserted BC * [WIP] Interpolation modes * Added InterpolationModes enum * Added supported for int values for interpolation for BC * Removed useless test code * Fix flake8

Deprecated arguments: resample and fillcolor

1b22ddc

Replaced by interpolation and fill

facebook-github-bot added the cla signed label Nov 3, 2020

datumbox reviewed Nov 3, 2020

View reviewed changes

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

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

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

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

vfdev-5 commented Nov 3, 2020

View reviewed changes

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

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

vfdev-5 mentioned this pull request Nov 3, 2020

Fill color support for tensor affine transforms #2904

Merged

vfdev-5 added 3 commits November 3, 2020 16:30

Updates according to the review

20c7177

Merge branch 'master' into vfdev-5/deprecate-resample-fillcolor

7f621a3

Added tests to check warnings and asserted BC

1bc499e

datumbox approved these changes Nov 4, 2020

View reviewed changes

vfdev-5 added 2 commits November 5, 2020 09:48

Merge branch 'master' into vfdev-5/deprecate-resample-fillcolor

f0d0257

Merge branch 'master' into vfdev-5/deprecate-resample-fillcolor

453722c

fmassa reviewed Nov 5, 2020

View reviewed changes

vfdev-5 added 5 commits November 10, 2020 13:31

Merge branch 'master' of github.com:pytorch/vision into vfdev-5/depre…

d9c48d6

…cate-resample-fillcolor

[WIP] Interpolation modes

665350e

Added InterpolationModes enum

ceadb65

Merge branch 'vfdev-5/deprecate-resample-fillcolor' of github.com:Qua…

e609722

…nsight/vision into vfdev-5/deprecate-resample-fillcolor

Merge branch 'master' into vfdev-5/deprecate-resample-fillcolor

ed1b509

vfdev-5 changed the title ~~Deprecated arguments: resample and fillcolor~~ Introduced InterpolationModes and deprecated arguments: resample and fillcolor Nov 16, 2020

vfdev-5 requested review from datumbox and fmassa November 16, 2020 16:25

datumbox reviewed Nov 17, 2020

View reviewed changes

torchvision/transforms/functional_pil.py Show resolved Hide resolved

datumbox reviewed Nov 17, 2020

View reviewed changes

torchvision/transforms/functional_tensor.py Show resolved Hide resolved

datumbox reviewed Nov 17, 2020

View reviewed changes

torchvision/transforms/functional.py Show resolved Hide resolved

vfdev-5 changed the title ~~Introduced InterpolationModes and deprecated arguments: resample and fillcolor~~ [BC-breaking] Introduced InterpolationModes and deprecated arguments: resample and fillcolor Nov 17, 2020

datumbox approved these changes Nov 17, 2020

View reviewed changes

fmassa requested changes Nov 18, 2020

View reviewed changes

vfdev-5 added 5 commits November 20, 2020 13:26

Added supported for int values for interpolation for BC

3768519

Merge branch 'master' into vfdev-5/deprecate-resample-fillcolor

6335042

Merge branch 'vfdev-5/deprecate-resample-fillcolor' of github.com:Qua…

4fb7790

…nsight/vision into vfdev-5/deprecate-resample-fillcolor

Removed useless test code

cf3d711

Fix flake8

6907505

vfdev-5 requested a review from fmassa November 20, 2020 14:15

fmassa approved these changes Nov 27, 2020

View reviewed changes

fmassa merged commit 0c44513 into pytorch:master Nov 27, 2020

vfdev-5 deleted the vfdev-5/deprecate-resample-fillcolor branch November 27, 2020 16:36

datumbox mentioned this pull request Nov 27, 2020

Rename InterpolationModes to InterpolationMode #3055

Merged

vfdev-5 mentioned this pull request Sep 27, 2021

Harmonize functional transforms arguments #2479

Closed

vfdev-5 mentioned this pull request Jan 17, 2023

[NOMRG] TransformsV2 questions / comments #7092

Closed

pmeier mentioned this pull request Feb 9, 2023

Revert removal and deprecation of int values for interpolation? #7205

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BC-breaking] Introduced InterpolationModes and deprecated arguments: resample and fillcolor #2952

[BC-breaking] Introduced InterpolationModes and deprecated arguments: resample and fillcolor #2952

vfdev-5 commented Nov 3, 2020 •

edited

Loading

codecov bot commented Nov 3, 2020 •

edited

Loading

datumbox left a comment

vfdev-5 left a comment

datumbox left a comment

fmassa left a comment

fmassa Nov 5, 2020 •

edited

Loading

vfdev-5 commented Nov 5, 2020 •

edited

Loading

vfdev-5 commented Nov 5, 2020 •

edited

Loading

fmassa commented Nov 5, 2020

vfdev-5 commented Nov 5, 2020

datumbox left a comment •

edited

Loading

vfdev-5 commented Nov 17, 2020 •

edited

Loading

fmassa left a comment

fmassa Nov 18, 2020

fmassa left a comment

fmassa Nov 27, 2020

datumbox commented Nov 27, 2020

vfdev-5 commented Nov 27, 2020

	"Argument resample is deprecated and will be removed since v0.10.0. Please, use interpolation instead"
	"Argument resample is deprecated and will be removed in v0.10.0. Please, use interpolation instead"

[BC-breaking] Introduced InterpolationModes and deprecated arguments: resample and fillcolor #2952

[BC-breaking] Introduced InterpolationModes and deprecated arguments: resample and fillcolor #2952

Conversation

vfdev-5 commented Nov 3, 2020 • edited Loading

codecov bot commented Nov 3, 2020 • edited Loading

Codecov Report

datumbox left a comment

Choose a reason for hiding this comment

vfdev-5 left a comment

Choose a reason for hiding this comment

datumbox left a comment

Choose a reason for hiding this comment

fmassa left a comment

Choose a reason for hiding this comment

fmassa Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

vfdev-5 commented Nov 5, 2020 • edited Loading

vfdev-5 commented Nov 5, 2020 • edited Loading

fmassa commented Nov 5, 2020

vfdev-5 commented Nov 5, 2020

datumbox left a comment • edited Loading

Choose a reason for hiding this comment

vfdev-5 commented Nov 17, 2020 • edited Loading

fmassa left a comment

Choose a reason for hiding this comment

fmassa Nov 18, 2020

Choose a reason for hiding this comment

fmassa left a comment

Choose a reason for hiding this comment

fmassa Nov 27, 2020

Choose a reason for hiding this comment

datumbox commented Nov 27, 2020

vfdev-5 commented Nov 27, 2020

vfdev-5 commented Nov 3, 2020 •

edited

Loading

codecov bot commented Nov 3, 2020 •

edited

Loading

fmassa Nov 5, 2020 •

edited

Loading

vfdev-5 commented Nov 5, 2020 •

edited

Loading

vfdev-5 commented Nov 5, 2020 •

edited

Loading

datumbox left a comment •

edited

Loading

vfdev-5 commented Nov 17, 2020 •

edited

Loading