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

Fixed tensor type issue in affinetransform and warp #189

Merged
merged 1 commit into from
Sep 13, 2016

Conversation

chsasank
Copy link
Contributor

Refer to #187.

Previously the following code would've given an error:

require 'torch'
require 'init'
torch.setdefaulttensortype('torch.FloatTensor')

local im = image.lena()
local matrix = torch.Tensor({{1,0.5},{0,1}}):double()
local im2 = image.affinetransform(im,matrix,'bilinear')

local fld = torch.zeros(2, 512, 512):double()
local im2 = image.warp(im,fld,'bilinear')

This seems to be because src.image.warp expects field to be of default tensor type. This issue is fixed by coercing passed matrix or field to default tensor type.

@soumith soumith merged commit aa635cf into torch:master Sep 13, 2016
@soumith
Copy link
Member

soumith commented Sep 13, 2016

Thanks!

@TimZaman
Copy link
Contributor

TimZaman commented Oct 4, 2016

If the default tensor type is double, but the image is a float, this PR will make it fail.
/Users/tzaman/torch/install/bin/luajit: /Users/tzaman/torch/install/share/lua/5.1/image/init.lua:966: bad argument #3 to 'warp' (torch.FloatTensor expected, got torch.DoubleTensor)
Reproduce:

require 'torch'
require 'image'
torch.setdefaulttensortype('torch.DoubleTensor')
pr = image.lena():float()
makes_me = torch.zeros(2, 512, 512):double()
sad = image.warp(pr,makes_me,'bilinear')

@chsasank
Copy link
Contributor Author

chsasank commented Oct 4, 2016

Hi @TimZaman ,
I think you are mistaken. This PR didn't modify anything related to wrap. It modified affinetransform. In fact it fixed the exact bug you've faced with warp except for affinetransform.

@TimZaman
Copy link
Contributor

TimZaman commented Oct 4, 2016

@chsasank , how about line 955?
local field = field:typeAs(torch.Tensor()) -- coerce matrix to default tensor type
Should have been
local field = field:typeAs(src) -- coerce matrix to src tensor type

@chsasank
Copy link
Contributor Author

chsasank commented Oct 4, 2016

That doesn't seem to work if src is not default tensor type.

This seems to be because src.image.warp expects field to be of default tensor type.

@TimZaman
Copy link
Contributor

TimZaman commented Oct 4, 2016

Do you agree that the above lua snippet a few comments back does not
currently work, and that this PR changes code (one line L955) for warp as
well?

On Tuesday, 4 October 2016, Sasank Chilamkurthy [email protected]
wrote:

That doesn't seem to work if src is not default tensor type.

This seems to be because src.image.warp expects field to be of default
tensor type.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#189 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AHXSRESpyDgxzwprV_8xd9fKZsyViwPfks5qwjDKgaJpZM4J7wfR
.

@chsasank
Copy link
Contributor Author

chsasank commented Oct 4, 2016

Yeah, that does seem to be the case. The PR seems to be a bad attempt to fix bug #187 . I'm still figuring out where exactly the error is raised. I'd appreciate any help.

One problem with local field = field:typeAs(src) is that src can be a CharTensor. That'll round field to nearest integer and therefore producing unexpected results.

@TimZaman
Copy link
Contributor

TimZaman commented Oct 4, 2016

It makes sense to introduce some code testing, also with the affineTransform. The warp function's argument-3 (field) has to be the same as the image's.
I will suggest a PR to fix this. If you want to use a byte or char-tensor, you can manually char:float():warp():char() it back to its original state. This wasn't possible before this PR either, and has never seemed to raise any issues.
I'll make a new PR and add a minor test too so that we know what we're going in the right direction.

@chsasank
Copy link
Contributor Author

chsasank commented Oct 4, 2016

The warp function's argument-3 (field) has to be the same as the image's.

It'd be nice if you can point where this check is enforced. I have gone through generic/image.c but couldn't find it.

soumith added a commit that referenced this pull request Oct 4, 2016
lukeyeager added a commit to lukeyeager/torch-distro that referenced this pull request Oct 4, 2016
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