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

Make shear operation area preserving #1529

Merged
merged 3 commits into from
Oct 29, 2019
Merged

Conversation

pedrofreire
Copy link
Contributor

This PR handles issue #1185.

PR #1371 attempted to fix this, but noticed the issue that the formula did not give the same shear direction - the reason for this is that the formulas in #1185 used a different sign for the shear.

So, to fix the problem, we just have to replace the signs of shear_x and shear_y in the matrix formula from #1185.

I verified the formula by hand, and by the following sympy code:

from sympy import Matrix, cos, sin, tan, simplify
from sympy.abc import x, y, phi

Xs = Matrix(
        [[1, -tan(x)],
         [0, 1]]
        )

Ys = Matrix(
        [[1, 0],
         [-tan(y), 1]]
        )

R = Matrix(
        [[cos(phi), -sin(phi)],
         [sin(phi), cos(phi)]]
        )

RSS = Matrix(
        [[cos(phi - y)/cos(y), -cos(phi - y)*tan(x)/cos(y) - sin(phi)],
         [sin(phi - y)/cos(y), -sin(phi - y)*tan(x)/cos(y) + cos(phi)]])

print(simplify(R * Ys * Xs - RSS))
# Matrix([[0, 0], [0, 0]])

(One thing I'm uncertain is whether there is really a gain of performance by avoiding to calculate the explicit matrices and inverses, since the matrices are small and numpy can be quite efficient - I might run some time comparisons to check.)

Pedro Freire added 2 commits October 28, 2019 18:20
The previous shear implementation did not preserve area, and we
implement a version that does.

The formula used was verified with the following sympy code:

from sympy import Matrix, cos, sin, tan, simplify
from sympy.abc import x, y, phi

Xs = Matrix(
        [[1, -tan(x)],
         [0, 1]]
        )

Ys = Matrix(
        [[1, 0],
         [-tan(y), 1]]
        )

R = Matrix(
        [[cos(phi), -sin(phi)],
         [sin(phi), cos(phi)]]
        )

RSS = Matrix(
        [[cos(phi - y)/cos(y), -cos(phi - y)*tan(x)/cos(y) - sin(phi)],
         [sin(phi - y)/cos(y), -sin(phi - y)*tan(x)/cos(y) + cos(phi)]])

print(simplify(R * Ys * Xs - RSS))

One thing that is not clear (and could be tested) is whether avoiding
the explicit products and calculations in _get_inverse_affine_matrix
really gives performance benefits - compared to doing the explicit
calculation done in _test_transformation.
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 for the PR!

I only really have one ask, which is to use np.matmul instead of @, because @ is not present in Python2. Apart from that, the rest is good to go, I only have minor comments.

test/test_transforms.py Outdated Show resolved Hide resolved
test/test_transforms.py Outdated Show resolved Hide resolved
torchvision/transforms/functional.py Outdated Show resolved Hide resolved
@@ -8,6 +8,7 @@
except ImportError:
accimage = None
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.

nit: Any reason to use numpy's functions instead of those from math?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No strong reason here - my reason for this import was not much of whether to use math or numpy, but more that I felt this type of code is very error prone, and it was worth it to import the names directly and make the formulae more readable. And then for importing from numpy instead of math, it was just because the numpy functions are a superset of the math ones in terms of behavior (i.e. np.sin works for normal numbers and for np.array while math won't work for np.array), in case the functions are needed for np.arrays in the future.

torchvision/transforms/functional.py Show resolved Hide resolved
The @ syntax is not supported in Python 2.
@codecov-io
Copy link

codecov-io commented Oct 29, 2019

Codecov Report

Merging #1529 into master will increase coverage by 0.03%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1529      +/-   ##
==========================================
+ Coverage    64.4%   64.44%   +0.03%     
==========================================
  Files          87       87              
  Lines        6746     6750       +4     
  Branches     1038     1038              
==========================================
+ Hits         4345     4350       +5     
+ Misses       2099     2097       -2     
- Partials      302      303       +1
Impacted Files Coverage Δ
torchvision/transforms/functional.py 71.67% <88.88%> (+0.61%) ⬆️

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 f612182...1bba842. Read the comment docs.

@pedrofreire pedrofreire requested a review from fmassa October 29, 2019 11:56
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!

@fmassa fmassa merged commit c226bb9 into pytorch:master Oct 29, 2019
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.

3 participants