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

[Feat] kornia.Resize has a bad default for align_corners #777

Closed
pmeier opened this issue Nov 10, 2020 · 9 comments
Closed

[Feat] kornia.Resize has a bad default for align_corners #777

pmeier opened this issue Nov 10, 2020 · 9 comments
Assignees
Labels
Difficulty: easy Describes the difficulty of an issue to be solved feature request New feature or request help wanted Extra attention is needed

Comments

@pmeier
Copy link
Contributor

pmeier commented Nov 10, 2020

馃悰 Bug

kornia.Resize has a bad default for align_corners if interpolation=="nearest" or interpolation=="area".

To Reproduce

import torch
import kornia

transform = kornia.Resize(32, interpolation="nearest")

input = torch.rand(1, 3, 16, 16)
transform(input)
ValueError: align_corners option can only be set with the interpolating modes: linear | bilinear | bicubic | trilinear

Expected behavior

This should work out of the box. You need to explicitly set align_corners=None to get this working. This is neither documented nor even accepted by the type annotations.

Environment

kornia==0.4.1

@pmeier
Copy link
Contributor Author

pmeier commented Nov 10, 2020

I only know that you guys had problems with align_corners before so here is my proposal to fix this:

def parse_align_corners(
    align_corners: Optional[bool], interpolation: str
) -> Optional[bool]:
    if align_corners is not None:
        return align_corners

    return (
        False
        if interpolation in ("linear", "bilinear", "bicubic", "trilinear")
        else None
    )

If you use that in the constructor of every transformation that has the align_corners parameter together with align_corners=None as default, you should be good. I imagine most users don't care about align_corners at all as long as it works.

@shijianjian
Copy link
Member

Yes, you are right. We should make a better default.

Should we set it to None directly for resize?

@pmeier
Copy link
Contributor Author

pmeier commented Nov 16, 2020

@shijianjian

Should we set it to None directly

I wouldn't recommend that:

import torch
import kornia

transform = kornia.Resize(32, interpolation="bilinear", align_corners=None)

input = torch.rand(1, 3, 16, 16)
transform(input)
UserWarning: Default upsampling behavior when mode=bilinear is changed to align_corners=False since 0.4.0. 
Please specify align_corners=True if the old behavior is desired. 
See the documentation of nn.Upsample for details.

To avoid the error and the warning, you need to set align_corners=False if interpolation in ("linear", "bilinear", "bicubic", "trilinear") and align_corners=None otherwise. That is exactly what the parse_align_corners function does that I suggested in #777 (comment).

for resize?

Again, I wouldn't recommend that. What is special about resize? Shouldn't this behavior consistent throughout the codebase? I would recommend doing something along the lines of

def resize(..., interpolation="bilinear", align_corners=None):
    align_corners = parse_align_corners(align_corners, interpolation)
    ...

in every user-facing function or module that takes the align_corners keyword.

@shijianjian
Copy link
Member

shijianjian commented Nov 17, 2020

I see. That is reasonable.

To avoid the error and the warning, you need to set align_corners=False if interpolation in ("linear", "bilinear", "bicubic", "trilinear") and align_corners=None otherwise. That is exactly what the parse_align_corners function does that I suggested in #777 (comment).

From what you proposed, this is a general setting that fits all functions with (align_corners, interpolation), right? I mean it is not only for resize.

@edgarriba
Copy link
Member

@pmeier do you think you can send a PR with it ? we are planning the release for v0.5 in a couple of weeks and this definitely can be good sanity patch. I guess this will also help with this #747

@edgarriba edgarriba changed the title kornia.Resize has a bad default for align_corners [Feat] kornia.Resize has a bad default for align_corners Nov 21, 2020
@edgarriba edgarriba added the feature request New feature or request label Nov 21, 2020
@pmeier
Copy link
Contributor Author

pmeier commented Dec 1, 2020

Sorry for the delay. I can give it a shot.

@edgarriba
Copy link
Member

@pmeier can you check #941 ? to see the next steps for augmentations and see hwo we could proceed with this ? /cc @shijianjian

@edgarriba edgarriba added Difficulty: easy Describes the difficulty of an issue to be solved help wanted Extra attention is needed labels Apr 5, 2021
@adamjstewart
Copy link
Contributor

adamjstewart commented Nov 27, 2022

This appears to be fixed in the latest release. The following has no errors:

transform = kornia.geometry.transform.Resize(32, interpolation="nearest")

although the original error can be reproduced with:

transform = kornia.geometry.transform.Resize(32, interpolation="nearest", align_corners=True)

@edgarriba
Copy link
Member

@pmeier shall we close this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Difficulty: easy Describes the difficulty of an issue to be solved feature request New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants