ConcatTable nested table input

[nicholas-leonard]

fixes #37

This PR adds supports for nested table inputs of arbitrary depth. Includes unit tests and doc.

[soumith]

@jonathantompson it's your job to review this :p

[jonathantompson]

This is picky, but I think ConcatTable should be able to switch between table and non-table inputs interchangeably. Going from non-table to table is OK with this PR, but going from table to non-table breaks:

require 'nn'
input = {torch.rand(1)}
m = nn.ConcatTable()
m:add(nn.Identity())
m:forward(input)
m:backward(input, m.output)
input = input[1]
m:forward(input)
m:backward(input, m.output)  -- Fails here

[nicholas-leonard]

Try switching m.table=false before going to Tensor mode.

[jonathantompson]

Yes, that works... But should the user have to do that manually? Do any
other container modules require consistent table / non-table inputs?
Sequential for instance doesn't require that you change self.table... I
feel like ConcatTable should have the same behaviour. What do you think?

On Wed, Jul 16, 2014 at 12:02 PM, Nicholas Léonard <notifications@github.com

wrote:

Try switching self.table=false before going to Tensor mode.

—
Reply to this email directly or view it on GitHub
#38 (comment).

[nicholas-leonard]

I think a Module should expect the general shape of and input/gradOutput to be the same. The only thing that should be expected to vary is the batchSize. If someone ever sees the need for mutable input/gradOutput shapes, they can always do a PR to change the module.

[jonathantompson]

Actually, I think most modules will reshape their outputs and gradInputs according to input tensor size changes. I think this is an important feature.

I agree that this is a weird case since we're talking about tensor to table input changes, which is not really a "size" change. So maybe this can be a special case exception to the rule.

Otherwise the rest of the commit looks OK.

[soumith]

I think a Module should expect the general shape of and input/gradOutput to be the same. The only thing that should be expected to vary is the batchSize.

This is not the general expectation. The module should be able to take in all valid inputs. The only general expectation is that a :backward call is not made before the :forward call is made.

We have to keep it this way because there are many use cases where you want to send in inputs of variable sizes.

[soumith]

For example, while doing detection on pyramidal inputs, after training a face detector network for 32x32 patches.

[nicholas-leonard]

You guys won't let go on this. So I added all the necessary changes to make it work for variable anything. If the table varies in depth or breadth, if it varies from table to tensor or vice-versa, if any tensor changes size, it works. Yes I am sometimes lazy.

[soumith]

:) nikopia! @jonathantompson let me know when you test it out/review the changes.

[jonathantompson]

Looks good to me.

can training() evaluate() and share() sit in nn.Module and be inherited? ie:

function Module:evaluate()
  if self.modules then 
    for i=1,#self.modules do
        self.modules[i]:evaluate()
     end
  end
end

Something like that? I'm not sure what we have in the other container classes. For instance, Module:type() has something like that:

function Module:type(type)
   -- find all tensors and convert them
   for key,param in pairs(self) do
      if torch.typename(param) and torch.typename(param):find('torch%..+Tensor') then
         self[key] = param:type(type)
      end
   end
   -- find submodules in classic containers 'modules'
   if self.modules then
      for _,module in ipairs(self.modules) do
         module:type(type)
      end
   end
   return self
end

Anyway, just a thought. Otherwise the PR looks good!

[jonathantompson]

Ohh, and we should probably rebase this down to one commit.

[soumith]

the rebasing is usually not needed, because github adds a single merge commit that holds all of the commits in the PR. (so if you want to revert the PR, you just revert that one commit)

[soumith]

oh yes, @nicholas-leonard is it compatible with :type() and :training()/:evaluate() being passed on downwards?

[soumith]

also, :share()

[soumith]

(I haven't looked at the code)

[jonathantompson]

"the rebasing is usually not needed, because github adds a single merge
commit that holds all of the commits in the PR. (so if you want to revert
the PR, you just revert that one commit)"

I didn't realize. Thanks!

On Thu, Jul 17, 2014 at 10:26 AM, Soumith Chintala <notifications@github.com

wrote:

also, :share()

—
Reply to this email directly or view it on GitHub
#38 (comment).

[nicholas-leonard]

:type() needed to be reimplemented due to self.gradInput sometimes being a Table, other times a Tensor.

[nicholas-leonard]

As for :training()/evaluate(), they could be refactored into Module for all container Modules, but that would be the subject of another PR, as all container Modules have these reimplemented.

[nicholas-leonard]

I didn't make any changes to :share(), so that should still work since no change has been made to the actual storing of composite Modules added through :add(). This PR only concerns the accumulation of gradInput Tables vs Tensors.

[soumith]

Thanks so much for the PR!!!