Add AffineTransformation #793
Conversation
update before transform
update 9/03/19
11/03/19
Add Affinetransformation to superseed LinearTransformation
torchvision/transforms/transforms.py
Outdated
| def __init__(self, transformation_matrix): | ||
| warnings.warn("The use of the transforms.LinearTransformation transform is deprecated, " + | ||
| "please use transforms.AffineTransformation instead.") | ||
| super(LinearTransformation, self).__init__(transformation_matrix) |
There was a problem hiding this comment.
I suppose you want to pass a mean_vector filled with zeros here?
There was a problem hiding this comment.
What would you prefer for the dimension of mean_vector: 1xD or Dx1 given that transformation_matrix is DxD?
There was a problem hiding this comment.
According to the implementation of LinearTransformation I think it should be 1 x D.
I feel that the doc of the application of LinearTransformation about whitening is broken. The doc only shows the 1st line of the paragraph in the application block and the rest outside it. How do we write a multi-line bullet point? Do we replace - with ---?
There was a problem hiding this comment.
@ekagra-ranjan the dimension of the tensor doesn't matter, you can pass a D tensor and it will be fine, given that broadcasting will happen and the tensor will be filled with all zeros anyway.
About the doc, I'm not sure about how to properly format things. I'd try compiling the documentation locally and checking it visually
11/03/19 6:41pm
Codecov Report
@@ Coverage Diff @@
## master #793 +/- ##
==========================================
+ Coverage 38.1% 38.13% +0.03%
==========================================
Files 32 32
Lines 3128 3136 +8
Branches 488 489 +1
==========================================
+ Hits 1192 1196 +4
- Misses 1857 1860 +3
- Partials 79 80 +1
Continue to review full report at Codecov.
|
|
@fmassa Is there anything else that you want me to do with this PR? |
|
@fmassa Are the changes fine? |
torchvision/transforms/transforms.py
Outdated
| if transformation_matrix.size(0) != transformation_matrix.size(1): | ||
| raise ValueError("transformation_matrix should be square. Got " + | ||
| "[{} x {}] rectangular matrix.".format(*transformation_matrix.size())) | ||
|
|
||
| if mean_vector.size(1) != transformation_matrix.size(0): |
There was a problem hiding this comment.
this is raise an error if the user tries to use LinearTransformation, because the mean vector is 1d.
I really think that you should just make mean_vector here be 1d as well, and change it in the documentation.
Also, can you keep the tests for the LinearTransformation?
torchvision/transforms/transforms.py
Outdated
| format_string += (str(self.transformation_matrix.numpy().tolist()) + ')') | ||
| format_string += (", (mean_vector=" + str(self.mean_vector.numpy().tolist()) + ')') |
There was a problem hiding this comment.
no need to convert to numpy here, tolist() also exists for torch tensors
|
Sorry if I misunderstood this, but I thought affine transformation means rotate, scale, shift, and shear, as shown in |
|
Hum, there seems to be indeed a problem with our naming, which will indeed make this very confusing Given that this is not in the 0.2.2 release, I'll rename this function with another name. |
|
I have 2 suggestions for the name:
|
|
@ekagra-ranjan I agree with leaving the name |
|
Sure. |
In reference to #569 added AffineTransformation to superseed LinearTransformation.