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

Change the default of antialias parameter to True #7093

Closed
NicolasHug opened this issue Jan 16, 2023 · 1 comment · Fixed by #7160
Closed

Change the default of antialias parameter to True #7093

NicolasHug opened this issue Jan 16, 2023 · 1 comment · Fixed by #7160

Comments

@NicolasHug
Copy link
Member

NicolasHug commented Jan 16, 2023

This issue proposes to change the default behaviour of antialias from None to True.


The Resize transform (and other related ones) accepts an antialias parameter. For historical reasons, the default behaviour of antialias is inconsistent across backends. By default, antialias is:

  • True for PIL images (PIL does not accept no-antialiasing)
  • False for Tensors (when the tensor backend was added, interpolate() did not support antialiasing. We had to introduce the parameter with False as as a default in orde to maintain backward compat of results).

This means that in the simple snippet below, you get two very different resized images depending on whether the input is a PIL image or a Tensor:

pil_img = Image.open(...)
tensor_img = to_tensor(pil_img)

out_pil = Resize()(pil_img)
out_tensor = Resize()(tensor_img)

This unfortunately trips up users (see e.g. this internal report), leads to sub-accurate models, and also leads to serious bugs like #6506 which we all missed during design / review.


Proposed migration path

  1. For all transforms / functionals that have the interpolate parameter, we change its current default from None to "warn" (or whatever). This "warn" value behaves exactly like None, but raises a warning indicating users to explicitly set either True, False or None. In 2 versions we can remove "warn" and do the switch to True.

Side note: It is worth keeping the None option: since PIL doesn't support no-antialiasing, antialias=False would raise an annoying warning when using it with PIL. This could typically be needed in training pipelines where multiple antialias strategies or backends are in use (something we may eventually do in torchvision....)

One issue with the proposed deprecation path is that the preset inference transforms (https://pytorch.org/vision/main/models.html#using-the-pre-trained-models) would start raising a warning that users have no control over:

weights = ResNet50_Weights.DEFAULT
preprocess = weights.transforms()

This would now raise a warning, but users can't disable it since they can't pass weights.transforms(antialias=True): those preset transforms don't have an antialias parameter even-though they rely on Resize() under the hood. Which brings me to

  1. Add an antialias parameter to all the preset transforms that rely on Resize(), and let users write:

    weights = ResNet50_Weights.DEFAULT
    preprocess = weights.transforms(antialias=True)

    In 2 versions when the migration is done, they can revert to their original code if they want to:

    weights = ResNet50_Weights.DEFAULT
    preprocess = weights.transforms()  # same as antialias=True

It's worth noting that those preset transforms were not originally intended to be customizable by users, i.e. users are not supposed to pass parameters. However, this is absolutely not enforced in the current API, and not documented/visible either. I would agree that 2. goes somewhat against the original design but a) this would only be for 2 versions and b) IMHO this is still better than the alternative, which is a silent change:

Alternative

An alternative to 2. is:

  1. Do a hard-breaking bug-fix: hard-code antialias=True in the the preset transforms code. This may be a bug-fix but this still results in a silent change.

I have a preference for 2. because 3. might be more disruptive: users would suddenly see their accuracy bump (which is usually a good thing), but we never know what might happen in the wild. I'd prefer avoiding the surprise effect and let them migrate gently. It's a little more work for us, but in two versions when the deprecation/migration is done, none of this will matter anyway.

@pmeier
Copy link
Collaborator

pmeier commented Jan 16, 2023

As discussed offline, +1 for 2.

It's worth noting that those preset transforms were not originally intended to be customizable by users, i.e. users are not supposed to pass parameters. However, this is absolutely not enforced in the current API, and not documented/visible either.

Should we enforce it then? Of course we need the antialias parameter if we go for 2., but can't we remove all the other ones? AFAIK (and please correct me here if I'm wrong), the parameters are not documented anywhere, right? We would need to somehow document the antialias one so that could make the whole thing even weirder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants