hughperkins / clnn

OpenCL backend for Torch nn neural networks library
BSD 2-Clause "Simplified" License
126 stars 16 forks source link

ClassNLLCriterion - add support for one dimensional input #33

Closed gloine closed 8 years ago

gloine commented 8 years ago

Hi,

I was following up the 60 mins Torch tutorial at:

https://github.com/soumith/cvpr2015/blob/master/Deep%20Learning%20with%20Torch.ipynb

and wanted to modify it to run with OpenCL instead of CUDA. The tutorial relies on the basic StochasticGradient module provided by nn package, which does not accept 2D minibatch input. So the clnn ClassNLLCriterion module refused to run with the error 'Input to clnn.ClassNLLCriterion should be 2-d tensor'. I really wanted to finish this tutorial using OpenCL, and ended up modifying the code. I believe it covers one dimensional case completely.

Please review the code and save people like me!

Best regards, Gloine

hughperkins commented 8 years ago

Hi Jaehyung, Thanks!

I've taken your changes, and added a unit test to them, and modified target from a scalar to 1-d tensor of length 1, to be consistent with cunn https://github.com/torch/cunn/blob/master/test.lua#L3300 . Please can you check my pull request into your branch, and merge that?

Once merged, I will merge your branch into master.

hughperkins commented 8 years ago

Thanks! Looks good :-)

gloine commented 8 years ago

Hi Hugh,

I have another problem. The merged code does not run my tutorial since trainset.label is not a cuda (or cl) tensor there. My surprise is that the cunn code does not seem to accept a number instead of cuda tensor. I don't have a cuda environment so cannot check whether the original tutorial is outdated or not.

I will setup a cuda environment somewhere and test the tutorial code...there should be something we are missing, if it runs as it is.

Thanks, Jaehyung

hughperkins commented 8 years ago

Ah, apparently you are right. In the nn ClassNLLCriterion, the first thing it does is create a tensor from the number, if it's a number:

   if type(target) == 'number' then
      if input:type() ~= 'torch.CudaTensor' then
         self.target = self.target:long()
      end
      self.target[1] = target

So, apparently your original PR was actually a valid use-case. Seems it should be able to handle both.

I'll take a look.

hughperkins commented 8 years ago

Try now?

gloine commented 8 years ago

Hmm...how do I pull your code?

gloine commented 8 years ago

Oh, it appears now

hughperkins commented 8 years ago

A couple of ways:

hughperkins commented 8 years ago

Ok :-)

gloine commented 8 years ago

It runs the tutorial code without problem now. Thank you!

hughperkins commented 8 years ago

Cool. Sorry about my temporarily breaking your perfectly valid fix :-)

gloine commented 8 years ago

No problems, I will add some unit tests next time I make a pull request. :)

hughperkins commented 8 years ago

:-)