Skip to content

Unified input for F.affine #2444

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

Merged
merged 7 commits into from
Jul 16, 2020
Merged

Conversation

vfdev-5
Copy link
Collaborator

@vfdev-5 vfdev-5 commented Jul 9, 2020

Related to #2292

Description:

  • Added code to unify inputs for F.affine
  • Added tests
  • Fixed incoherence in F_pil.affine with center of image (BC-breaking change?)
    • image rotated by 90 degrees is now exactly the same as the output image of torch.rot90 or F_t.affine
      => adapted corresponding tests on PIL images.

@vfdev-5 vfdev-5 marked this pull request as ready for review July 9, 2020 13:33
@vfdev-5 vfdev-5 changed the title Unified input for Affine Unified input for F.affine Jul 9, 2020
@vfdev-5 vfdev-5 mentioned this pull request Jul 9, 2020
16 tasks
@vfdev-5 vfdev-5 force-pushed the vfdev-5/issue-2292-affine branch from d7ae44c to f33b391 Compare July 9, 2020 15:43
Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Test failures seems related, but otherwise the PR looks good to me.

@@ -1,12 +1,11 @@
import math
import numbers
import warnings
from collections.abc import Iterable
from typing import Any
from typing import Any, Optional, Sequence

import numpy as np
from numpy import sin, cos, tan
Copy link
Member

Choose a reason for hiding this comment

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

maybe this import is not needed anymore?

# Accept 3 wrong pixels
self.assertLess(n_diff_pixels, 3,
# Accept 7 wrong pixels
self.assertLess(n_diff_pixels, 7,
Copy link
Member

Choose a reason for hiding this comment

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

Was this test flaky before? Or is this needed due to the change in the center of the transform pixel (the +0.5 is added in a different location now)


if need_cast:
# it is better to round before cast
img = torch.round(img).to(out_dtype)
Copy link
Member

Choose a reason for hiding this comment

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

I saw you did this round for a couple of functions, does it make results closer to PIL?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, rounding makes sense for example when floating results is

tensor([[ 0.09,  1.01,  1.97,  2.96,  3.99],
        [10.94, 12.15, 13.35, 14.56, 15.77],
        [22.92, 24.13, 25.34, 26.54, 27.75],
        [34.90, 36.11, 37.32, 38.53, 39.73],
        [46.88, 48.09, 49.30, 50.51, 51.72]])

and so casting without round gives

tensor([[ 0,  1,  1,  2,  3],
        [10, 12, 13, 14, 15],
        [22, 24, 25, 26, 27],
        [34, 36, 37, 38, 39],
        [46, 48, 49, 50, 51]], dtype=torch.uint8)

@vfdev-5 vfdev-5 requested a review from fmassa July 16, 2020 09:21
Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

Thanks a lot @vfdev-5 !

@fmassa fmassa merged commit 5f4b579 into pytorch:master Jul 16, 2020
@vfdev-5 vfdev-5 deleted the vfdev-5/issue-2292-affine branch July 16, 2020 09:33
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 this pull request may close these issues.

2 participants