search

lisa-lab / pylearn2

Warning: This project does not have any current developer. See bellow.
BSD 3-Clause "New" or "Revised" License
2.76k stars 1.09k forks source link

Copy-paste TheanoLinear's 2D convolution and give it an interface for not needing a batch size #177

Open goodfeli opened 11 years ago

goodfeli commented 11 years ago

Currently we subclass TheanoLinear's 2D convolution and add a few features to it. I think we should just copy-paste it into pylearn2.linear (giving the original source proper attribution of course) and give it a bit more of an overhaul. The main issue needing fixing is that right now it seems to require that the batch size be hardcoded. I don't think the underlying theano op requires that. We should check that this is the case, and if so, modify the interface to let the batch size vary.

nouiz commented 11 years ago

Theano ops do not request hard coded shape. A few ops take that optionally for optimization.

I don't know pylearn2.linear, but if it unroll graph, do reshape or call scan with fixed number of iteration, that could explain why it need hard coded shape.

goodfeli commented 11 years ago

@nouiz , this is caused by TheanoLinear, not pylearn2.linear. https://github.com/lisa-lab/pylearn2/blob/master/pylearn2/packaged_dependencies/theano_linear/conv2d.py

There shouldn't be any scan or graph unrolling, all this does is convolution and convolution transpose. I'm assuming @jaberg just wanted to discourage use of the non-hardcoded case because it runs slower. @jaberg -- there aren't any comments, do you remember why the image shape argument is required instead of defaulting to None? Do you remember if it will work if we pass in None for that required argument? I haven't tried to test it because this is not something I actively need myself, just something I've noticed is causing trouble for other people.

jaberg commented 11 years ago

Definitely worth a try putting in None. I recall it was important to hard-code the filterbank shape so that the transform object would know how to do both right-multiplication and left-multiplication, but I don't see why the number of vectors being right-multiplied or left-multiplied should need to be specified.

On Thu, Mar 14, 2013 at 12:08 AM, Ian Goodfellow notifications@github.comwrote:

@nouiz https://github.com/nouiz , this is caused by TheanoLinear, not pylearn2.linear. https://github.com/lisa-lab/pylearn2/blob/master/pylearn2/packaged_dependencies/theano_linear/conv2d.py

There shouldn't be any scan or graph unrolling, all this does is convolution and convolution transpose. I'm assuming @jaberghttps://github.com/jabergjust wanted to discourage use of the non-hardcoded case because it runs slower. @jaberg https://github.com/jaberg -- there aren't any comments, do you remember why the image shape argument is required instead of defaulting to None? Do you remember if it will work if we pass in None for that required argument? I haven't tried to test it because this is not something I actively need myself, just something I've noticed is causing trouble for other people.

— Reply to this email directly or view it on GitHubhttps://github.com/lisa-lab/pylearn2/issues/177#issuecomment-14885701 .

goodfeli commented 11 years ago

It looks like if we construct an image shape argument with an explicit None for the batch size it works.

lamblin commented 11 years ago

@goodfeli Does that mean this ticket is solved? Should we assign someone to work on that during summer?