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
[Bug] warp_perspective does not return the input if used with identity matrix #747
Comments
@pmeier I can confirm that happens the same in 0.4.0, 0.3.2, 0.3.1 but no in 0.3.0 |
@pmeier See: https://github.com/kornia/kornia/blob/master/kornia/geometry/transform/imgwarp.py#L48 it's now set False by default. This definitely needs better testing: https://github.com/kornia/kornia/blob/master/test/geometry/transform/test_imgwarp.py#L12 |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions, and happy coding day 😎 |
@edgarriba This should not be closed by the bot, since the bug won't go away if no one has posted anything new. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions, and happy coding day 😎 |
@pmeier When transform is "non-descructive", i.e. results in sampling ONLY integer locations from original image, like identity, rotation by 90 degrees, shift by integer number of pixels, the optimal and correct way is to use "align_corners=True", so the data is not changed. The probably best solution, would be to detect which kind of transform we get by checking the warp matrix, but it is non-trivial to implement. We will be glad to receive help here :) In the mean time, I would recommend you to send "align_corners" flag manually. |
@ducha-aiki Thanks for the explanation. Given that this leads to a lot of confusion I suggest that you add explanation probably with more details / examples to the documentation so that users can actually make an informed choice on how to set |
I agree with @pmeier that we might add it in the docs to let the users more aware of the differences of the parameter choice. This pic is a nice demonstration. |
Probably a separate page as a side note in the docs would fit and link functions to that. @ducha-aiki or @shijianjian could you go fo this? |
Probably adding to the homepage of the geometry module. https://kornia.readthedocs.io/en/latest/geometry.html# |
I don't see how this issue has been resolved---doesn't the change of defaults to Consider the original scenario of warping with the identity matrix and For kornia/kornia/geometry/transform/imgwarp.py Line 175 in 26db095
For kornia/kornia/geometry/transform/imgwarp.py Lines 100 to 104 in 26db095
Basically Kornia is not applying the "correction" for when |
@anibali possibly yes - what would you suggest here ? |
Our initial intention here was to mimic opencv behaviour. In the early stages, pytorch had default This is possibly one of the most core functionalities in the lib so I’d love to hear opinions and use cases on that end. |
Because I can't help myself I dug into the code and figured out what was going on. The issue is that your code incorrectly uses src = torch.tensor([
[10., 20, 30],
[40, 50, 60],
[70, 80, 90],
])[None, None]
points_1 = torch.tensor([[-1., -1.], [1., 1.]])[None, None]
points_2 = torch.tensor([[-2/3, -2/3], [2/3, 2/3]])[None, None]
sample_1 = F.grid_sample(src, points_1, align_corners=True)
sample_2 = F.grid_sample(src, points_2, align_corners=False)
print(sample_1[0, 0, 0, :]) # tensor([10., 90])
print(sample_2[0, 0, 0, :]) # tensor([10., 90]) Another way to put it: the tl;dr Does the concept of I don't think it does. We can calculate both Here's a code snippet showing thissrc = torch.tensor([
[ 0, 2, 4,],
[ 2, 4, 6,],
[ 4, 6, 8,],
], dtype=torch.float32)
src_th = src[None, None]
size = np.array(src.shape)
h, w = size
# as torch.nn.Upsample(..., align_corners=False)
M_F = torch.tensor([
[2, 0, 0.5],
[0, 2, 0.5],
[0, 0, 1],
], dtype=torch.float32).unsqueeze(0)
th_upsample_F = torch.nn.Upsample([*(size*2)], mode='bilinear', align_corners=False)(src_th)
kornia_as_th_F = kornia.geometry.warp_perspective(
src_th, M_F, size*2,
padding_mode='border',
align_corners=True)
# as torch.nn.Upsample(..., align_corners=True)
R = (2*3-1)/2 # I don't know the true formula for this...
M_T = torch.tensor([
[R, 0, 0],
[0, R, 0],
[0, 0, 1],
], dtype=torch.float32).unsqueeze(0)
th_upsample_T = torch.nn.Upsample([*(size*2)], mode='bilinear', align_corners=True)(src_th)
kornia_as_th_T = kornia.geometry.warp_perspective(
src_th, M_T, size*2,
padding_mode='reflection',
align_corners=True)
print(' == align_corners=False ==')
print(' - Torch upsample')
print(th_upsample_F[0, 0])
print(' - Kornia warp_perspective')
print(kornia_as_th_F[0, 0])
print(' == align_corners=True ==')
print(' - Torch upsample')
print(th_upsample_T[0, 0])
print(' - Kornia warp_perspective')
print(kornia_as_th_T[0, 0])
The purpose of |
@Multihuntr thanks for long detailed explanation. One possible thing I see here would be allowing |
@edgarriba I apologise for the long detailed explanation being overly long and detailed and possibly hard to read. I'm still straightening this out in my own head. 😅 Conceptually Note 1: I disagree with ducha-aiki. The blue dots diagram they shared is for Note 2: There are more than two possible interpretations of "upsample my image by 2", but all possible interpretations can be expressed using the other existing arguments ( |
@Multihuntr assuming that possibly makes sense to get rid of @pmeier @anibali @ducha-aiki any opinions about this deprecation ? I think that we introduced support to |
I agree with the deprecation. It is further supported by the fact that no other library seems to have
In my opinion the addition of an |
thanks so much - I'll open a separated issue to track the deprecation of |
🐛 Bug
The output of
kornia.geometry.warp_perspective
does not equal the input if the identity matrix is used.To Reproduce
Steps to reproduce the behavior:
Expected behavior
Environment
kornia==0.4.1
The text was updated successfully, but these errors were encountered: