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

problem in rotate_impl #29

Closed
skeydan opened this issue Nov 11, 2020 · 6 comments
Closed

problem in rotate_impl #29

skeydan opened this issue Nov 11, 2020 · 6 comments

Comments

@skeydan
Copy link
Contributor

skeydan commented Nov 11, 2020

The matrix returned from get_inverse_affine_matrix is 1-dimensional, and so cannot be reshaped like this:

> img <- torch_randint(0, 256, 16)$view(c(4,4))
> center_f <- c(0.0, 0.0)
> angle <- 180
> matrix <- get_inverse_affine_matrix(center_f, -angle, c(0.0, 0.0), 1.0, c(0.0, 0.0))
> theta <- torch::torch_tensor(matrix)$reshape(1, 2, 3)
Error in o(x, x$private, ...) : unused arguments (2, 3)
> matrix
[1] -1.000000e+00 -1.224647e-16  0.000000e+00  1.224647e-16
[5] -1.000000e+00  0.000000e+00

Would have liked to create a PR but this gets rather complex - in Python the value returned from _get_inverse_affine_matrix is a list as well, but it seems that in further processing we're doing something differently from Python:

# https://github.com/pytorch/pytorch/blob/74b65c32be68b15dc7c9e8bb62459efbfbde33d8/aten/src/ATen/native/
# AffineGridGenerator.cpp#L18
# Difference with AffineGridGenerator is that:
# 1) we normalize grid values after applying theta
# 2) we can normalize by other image size, such that it covers "extend" option like in PIL.Image.rotate
gen_affine_grid <- function(theta, w, h, ow, oh) {

@dfalbel dfalbel mentioned this issue Nov 11, 2020
@dfalbel
Copy link
Member

dfalbel commented Nov 11, 2020

#29 will probably fix. Can you send the code you are running so I can add a regression test?

@skeydan
Copy link
Contributor Author

skeydan commented Nov 12, 2020

Not yet fixed, but I wanted to continue on that, which ended up in a separate PR building on yours because I wasn't able to push to your branch - hope that's ok ... #31 ... unfortunately that doesn't fix it yet (((but personally I'm happy to see that I'm not the only one who gets confused over whether to use c(x, y, z) or just (x, y, z) Python-style...

Here is some test code - 2 versions, on different levels of abstraction:

# low-level
img <- torch_randint(0, 256, 16)$view(c(4,4))
center_f <- c(0.0, 0.0)
angle <- 180
matrix <- get_inverse_affine_matrix(center_f, -angle, c(0.0, 0.0), 1.0, c(0.0, 0.0))
rotate_impl(img, matrix=matrix, resample=0, expand = FALSE, fill = NULL)

# high-level
img <- torch_randint(0, 256, 16)$view(c(4,4))
angle <- 180
transform_rotate(img, angle)

@dfalbel
Copy link
Member

dfalbel commented Nov 12, 2020

I am not sure it's supposed to work with 2d tensors. Images in torch are represented by 3d tensors in the CHW. I added tests to your PR.

@skeydan
Copy link
Contributor Author

skeydan commented Nov 12, 2020

yeah me neither ... but I checked, it failed with 3d as well ... - although it's true, the error is different ...

@dfalbel
Copy link
Member

dfalbel commented Nov 12, 2020

OK, should e working with the last fix I pushed to your PR now

@skeydan
Copy link
Contributor Author

skeydan commented Nov 12, 2020

it does, - awesome!

@dfalbel dfalbel closed this as completed Nov 12, 2020
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 a pull request may close this issue.

2 participants