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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mismatch in docs and behavior of align_corners for nn.functional.interpolate #42729

Open
ORippler opened this issue Aug 7, 2020 · 5 comments
Assignees
Labels
module: docs Related to our documentation, both in docs/ and docblocks module: nn Related to torch.nn triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Comments

@ORippler
Copy link

ORippler commented Aug 7, 2020

馃摎 Documentation

In the docs of nn.functional.interpolate it is stated that align_corners is a bool with default value False. Furthermore, it is stated that align_corners does not affect interpolation modes 'area' as well as 'nearest' .

If you look at the implementation however, default of align_corners is None. Furthermore, when passing a bool for align_corners together with interpolation modes 'nearest' or 'area' to nn.functional.interpolate, a ValueError is raised

There is thus a clear mismatch between docstrings and implemented behavior of align_corners in combination with modes 'nearest' and 'area' for nn.functional.interpolate. Based on the intended behvaior, either the code or the docstrings should be changed.

Related PR that introduced align_corners and may clarify originally intended behavior: #5927

I would vote for changing the code to adhere to the behavior outlined in the docstrings, and am up to fixing the issue regardless of the made decision.

cc @jlin27 @albanD @mruberry

@ezyang ezyang added module: docs Related to our documentation, both in docs/ and docblocks module: nn Related to torch.nn triage review labels Aug 7, 2020
@ezyang
Copy link
Contributor

ezyang commented Aug 7, 2020

sending to triage review for some initial opinions

@ezyang
Copy link
Contributor

ezyang commented Aug 10, 2020

This is a side effect of 0.3.1 bc compatibility:

    .. warning::
        With ``align_corners = True``, the linearly interpolating modes
        (`linear`, `bilinear`, and `trilinear`) don't proportionally align the
        output and input pixels, and thus the output values can depend on the
        input size. This was the default behavior for these modes up to version
        0.3.1. Since then, the default behavior is ``align_corners = False``.
        See :class:`~torch.nn.Upsample` for concrete examples on how this
        affects the outputs.

0.3.1 is ancient news, so yes, let's just change the default behavior (change code).

@ezyang
Copy link
Contributor

ezyang commented Aug 10, 2020

@ailzhang will double check here, but it looks like we can change the behavior (and keep the docs as is.)

@ailzhang ailzhang self-assigned this Aug 10, 2020
@smessmer smessmer added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Aug 10, 2020
@ailzhang
Copy link
Contributor

@ORippler Yea for the default value of align_corners, we made it default to None just to notify the BC breaking change from align_corners True -> False in 0.4.0. As @ezyang said 0.4 is pretty old let's just change the code and make the default value to False :D
For align_corners with nearest/area mode, the code is correct that it's not allowed to set align_corners with nearest/area mode. So I'd suggest we update the docstring to make it clear.
Let us know whether this make sense to you, and please feel free to send a PR to fix! Thanks!

@ORippler
Copy link
Author

ORippler commented Aug 12, 2020

Hi @ezyang @ailzhang ,

your changes do make sense to me, however are also a BC breaking API change. In the current version, you can call functional.interpolate with align_corners = None and all interpolation modes, and align_corners is changed to False as required together with issuing the UserWarning before passing it to the internals. With your suggestion, one would now need to explicitly specify align_corners = None when calling functional.interpolate with modes nearest/area, which was not the case before.

Furthermore, the identical design paradigm (default align_corners = None, catch None to raise Warning, change value to False) is also used in grid_sample and affine_grid to denote identical behavioral changes made in Pytorch 1.3.0. For these functions, default values are also wrongly stated in docstrings to be False.

Imo, a consistent deprecation and API should be chosen to cover all functions that use align_corners and the underlying grid alignment functionality.
As GridSampler employed by grid_sample requires for align_corners to be True/False also in nearest mode, I would suggest to leave the current paradigm in place so long as we want to keep the UserWarning to denote the changed behavior.
We should instead no longer require align_corners = None for modes nearest/area inside nn.functional.interpolate to make it consistent with grid_sample and GridSampler. As an alternative, we could raise a warning reiterating that align_corners has no effect/is unused when align_corners != None and clarify documentation in the docstrings.

When the UserWarning of the changed behavior is removed, default value of align_corners should just be changed to False in all methods/functions related to this.

What do you think about my suggestion? Note that I am unfamiliar with the Pytorch internals and may have missed something obvious.

Offnote: why is align_corners and the underlying functionality used in GridSampler employed by grid_sample even in nearest mode yet unused/ignored in torch._C._nn.upsample_nearest2d ? Couldn't this possibly introduce inhomogeneieties in interpolation results ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: docs Related to our documentation, both in docs/ and docblocks module: nn Related to torch.nn triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module
Projects
None yet
Development

No branches or pull requests

4 participants