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

Shear Paramterization Fix #1371

Closed
wants to merge 3 commits into from
Closed

Conversation

ekagra-ranjan
Copy link
Contributor

Fixes the bug mentioned in #1185 using the solution proposed by @jacobhinkle.

@ekagra-ranjan
Copy link
Contributor Author

ekagra-ranjan commented Sep 25, 2019

@fmassa @vfdev-5 @ptrblck @jacobhinkle I did as agreed upon in #1185 but the result doesn't seem to consistent with the original shear. For e.g., using

RSS = [ cos(a+shear_y)/cos(shear_y)  cos(a+shear_y)tan(shear_x)/cos(shear_y) - sin(a) ]
      [ sin(a+shear_y)/cos(shear_y)  sin(a+shear_y)tan(shear_x)/cos(shear_y) + cos(a) ]
  

leads to shearing in -ve x-axis sense. When I changed it to:


RSS = [ cos(a+shear_y)/cos(shear_y)  cos(a+shear_y)tan(-shear_x)/cos(shear_y) - sin(a) ]
      [ sin(a+shear_y)/cos(shear_y)  sin(a+shear_y)tan(shear_x)/cos(shear_y) + cos(a) ]
 

the shearing in x-axis becomes consistent with the original implementation. However, I am not sure whether it is the right thing to do and if RSS still needs to be modified to behave properly. What form of RSS should be chosen for this?

@ekagra-ranjan
Copy link
Contributor Author

ekagra-ranjan commented Sep 25, 2019

Also, from the code it seems shear in x-axis is denoted by shear[0] whereas shearing in y-axis is given by shear[1]. Please correct me if I am wrong in this regard.

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 @ekagra-ranjan !

However, I am not sure whether it is the right thing to do and if RSS still needs to be modified to behave properly. What form of RSS should be chosen for this?

I didn't check the math yet, and I'd like to refer to @jacobhinkle to chime on that first.

Also, from the code it seems shear in x-axis is denoted by shear[0] whereas shearing in y-axis is given by shear[1]. Please correct me if I am wrong in this regard.

From Wolfram Alpha simplifications for the inverse of the RSS matrix (in the current format in torchvision), it seems indeed that shear[0] denotes the x axis, and shear[1] the y axis.

@@ -41,12 +41,9 @@ def _is_numpy_image(img):

def to_tensor(pic):
"""Convert a ``PIL Image`` or ``numpy.ndarray`` to tensor.

Copy link
Member

Choose a reason for hiding this comment

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

Can you revert those space changes? It will make the documentation render incorrectly.

@fmassa
Copy link
Member

fmassa commented Oct 29, 2019

Superseeded by #1529

Thanks @ekagra-ranjan for the original PR!

@fmassa fmassa closed this 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.

2 participants