Transposed Convolution Layers from base class #850

frapa · 2017-06-17T17:16:14Z

I made a bit of mess with the rebase (first time trying it...). I tried to merge the changes meaningfully. I'm uploading it because today I have no more time, then on Monday I'll see...

frapa · 2017-06-17T21:29:43Z

I really do not understand the error message I get from Travis... some import problem. The funny thing is on my pc py.test runs fine.

f0k

Thank you, looks good in principle! And sorry for the extra work of rebasing. Apart from the comments I have, the tests will need to be refactored as well to test all three dimensionalities. Check how it was done for the forward convolutional layers, and let me know if you need help. Cheers!

f0k · 2017-06-21T16:35:37Z

docs/modules/layers.rst

 Deconv2DLayer
+ Deconv3DLayer


Oh, good catch, we missed to add it there in #841.

I think I'd prefer the order:

TransposedConv1DLayer Deconv1DLayer TransposedConv2DLayer Deconv2DLayer TransposedConv3DLayer Deconv3DLayer

So the alias always comes directly after a documented layer.

f0k · 2017-06-21T16:36:16Z

docs/conf.py

@@ -354,6 +354,9 @@ def setup(app):
 if not hasattr(theano.tensor.nnet, 'conv3d'):
 theano.tensor.nnet.conv3d = Mock()
 reload(lasagne.layers.conv)
+if not hasattr(theano.tensor.nnet.abstract_conv, 'AbstractConv_gradInputs'):
+ theano.tensor.nnet.abstract_conv.AbstractConv1d_gradInputs = Mock()


I guess you meant AbstractConv_gradInputs, without 1d.

f0k · 2017-06-21T16:38:47Z

docs/modules/layers/conv.rst


 .. autoclass:: Deconv2DLayer

+.. autoclass:: Deconv3DLayer
+


Again, thank you for catching this, but I'd prefer to have each alias come directly after the actual class.

f0k · 2017-06-21T16:40:53Z

lasagne/layers/conv.py

 "Deconv2DLayer",
+ "Deconv3DLayer",
 "DilatedConv2DLayer",
 "TransposedConv3DLayer",
 "Deconv3DLayer",


The 3Ds are duplicated now. I'm fine with placing them before the dilated layers, but again I'd prefer the aliases directly after the respective classes.

f0k · 2017-06-21T16:46:13Z

lasagne/layers/conv.py

 conved = op(self.W, input, output_size)
 return conved

+
+class TransposedConv1DLayer(BaseTransposedConvLayer): # pragma: no cover


Detail: We usually (hopefully always) have two spaces before line comments.

f0k · 2017-06-21T17:02:24Z

lasagne/layers/conv.py

- the input (by adding ``stride - 1`` zeros between adjacent input elements),
- padding it with ``filter_size - 1 - crop`` zeros, and cross-correlating it
- with the filters. See [1]_ for more background.
+ See `BaseTransposedConvLayer` for more.


Aha, I guess we will have to copy this note to all subclasses, as we cannot refer to the base class.

f0k · 2017-06-21T17:03:40Z

lasagne/layers/conv.py


 Examples
 --------
 To transpose an existing convolution, with tied filter weights:

 >>> from lasagne.layers import Conv3DLayer, TransposedConv3DLayer
- >>> conv = Conv3DLayer((None, 1, 32, 32, 32), 16, 3, stride=2, pad=2)
+ >>> conv = Conv3DLayer((None, 1, 32, 64, 64), 16, 3, stride=2, pad=2)


I think the previous one was okay.

f0k · 2017-06-21T17:06:51Z

lasagne/layers/conv.py

- .. [1] Vincent Dumoulin, Francesco Visin (2016):
- A guide to convolution arithmetic for deep learning. arXiv.
- http://arxiv.org/abs/1603.07285,
- https://github.com/vdumoulin/conv_arithmetic


Ah, I think we'll have to replicate this reference in all three subclasses as well.

f0k · 2017-06-21T17:07:17Z

lasagne/layers/conv.py

- def __init__(self, incoming, num_filters, filter_size,
- stride=(1, 1, 1), crop=0, untie_biases=False,
+ def __init__(self, incoming, num_filters, filter_size, stride=1,
+ crop=0, untie_biases=False,


I'd prefer (1, 1, 1) just for consistency with the forward convolutional layers.

f0k · 2017-06-21T17:11:05Z

lasagne/layers/conv.py

+ # Hide TransposedConv2DLayer for old Theano versions
+ del TransposedConv2DLayer, Deconv2DLayer
+ __all__.remove('TransposedConv2DLayer')
+ __all__.remove('Deconv2DLayer')


The 2D version is always available in the Theano versions we support. I'd prefer to only add the check for 1D, directly after the 1D definitions.

f0k · 2017-06-21T17:17:18Z

I really do not understand the error message I get from Travis... some import problem.

The exact error is AttributeError: 'module' object has no attribute 'TransposedConv3DLayer', thrown on from .conv import *. A * import will import all names defined in __all__. I can't spot the mistake either; it should be removed from __all__ just like before your PR.

The funny thing is on my pc py.test runs fine.

Even with Theano 0.8?

frapa · 2017-07-04T15:59:52Z

No, maybe theano 0.8 is the problem then. I tried with the bleeding-edge version.

I fixed all the things you mentioned (thanks a lot for the in-depth review). I'm sorry for the many mistakes but it's my first time rebasing and I made a mess...

frapa · 2017-07-04T16:21:01Z

Ok, I tried adding test by copying and modifying the tests for the other cases. I am not at all sure how this whole unit-test thing works (I understand that it should check the output shape, but the code is a bit weird and right now I do not have time to delve into it.)

f0k

Thanks for the updates, and sorry for the thesis-based delay. Looks much better, there are still a few inconsistencies in the docstrings from copying things around, and the tests need to skip the TransposedConv1DLayer if it's not available. Otherwise the tests look good. It might be possible to refactor them as well to minimize code duplication, but we can leave that for another time (it's actually good to first check that the old tests still run through with the new code).

f0k · 2017-07-31T13:38:29Z

lasagne/tests/layers/test_conv.py

+ @pytest.mark.parametrize(
+ "input, kernel, output, kwargs", list(transp_conv1d_test_sets()))
+ def test_defaults(self, DummyInputLayer, input, kernel, output, kwargs):
+ from lasagne.layers import TransposedConv1DLayer


You'll need to wrap this in a try..except:

try: from lasagne.layers import TransposedConv1DLayer except ImportError: pytest.skip("TransposedConv1DLayer not available")

Same for the second test below. This ensures it will run through on Travis / on Theano 0.8.x.

f0k · 2017-07-31T13:40:52Z

lasagne/layers/conv.py

+ The dimensionality of the transposed convolution (i.e., the number of
+ spatial dimensions of each feature map and each convolutional filter).
+ If ``None``, will be inferred from the input shape.
+


The untie_biases docstring still needs to be adapted (https://github.com/Lasagne/Lasagne/pull/850/files#r123304501). Unfortunately I cannot comment on the correct line for that because it's not part of the diff.

f0k · 2017-07-31T13:42:24Z

lasagne/layers/conv.py

 W=lasagne.init.GlorotUniform(), b=lasagne.init.Constant(0.),
- nonlinearity=lasagne.nonlinearities.rectify, flip_filters=False, **kwargs)
+ nonlinearity=lasagne.nonlinearities.rectify, flip_filters=False,
+ n=None, **kwargs)


output_size is mysteriously missing here (not your fault, but would be good to fix while we're at it, should come before n)

This was also missing from the three subclasses. Fixed.

f0k · 2017-07-31T13:47:24Z

lasagne/layers/conv.py

+ incoming : a :class:`Layer` instance or a tuple
+ The layer feeding into this layer, or the expected input shape. The
+ output of this layer should be a 4D tensor, with shape
+ ``(batch_size, num_input_channels, input_rows, input_columns)``.


should be 3D, without input_columns

f0k · 2017-07-31T13:47:38Z

lasagne/layers/conv.py

+ An integer or 1-element tuple specifying the size of the filters.
+
+ stride : int
+ An integeror 1-element tuple specifying the stride of the


integeror -> integer or

f0k · 2017-07-31T13:57:00Z

lasagne/layers/conv.py

+ W : Theano shared variable, expression, numpy array or callable
+ Initial value, expression or initializer for the weights.
+ These should be a 4D tensor with shape
+ ``(num_input_channels, num_filters, filter_rows, filter_columns)``.


This is correct now. I wonder whether my previous comment (https://github.com/Lasagne/Lasagne/pull/850/files#r123304988) was originally attached to the 1D version and github messed up, or whether I accidentally thought it was about the 1D version.

f0k · 2017-07-31T13:58:47Z

lasagne/layers/conv.py

+ ``None``, the layer will have no biases. Otherwise, biases should be
+ a 1D array with shape ``(num_filters,)`` if `untied_biases` is set to
+ ``False``. If it is set to ``True``, its shape should be
+ ``(num_filters, output_rows, output_columns)`` instead.


Still open (https://github.com/Lasagne/Lasagne/pull/850/files#r123305116).

f0k · 2017-07-31T13:59:47Z

lasagne/layers/conv.py

- ``(batch_size, num_input_channels, input_depth, input_rows,
- input_columns)``.
+ output of this layer should be a 4D tensor, with shape
+ ``(batch_size, num_input_channels, input_rows, input_columns)``.


Still open (https://github.com/Lasagne/Lasagne/pull/850/files#r123307232).

f0k · 2017-07-31T14:00:35Z

lasagne/layers/conv.py

@@ -1006,7 +1325,8 @@ class TransposedConv3DLayer(BaseConvLayer): # pragma: no cover
 W : Theano shared variable, expression, numpy array or callable
 Initial value, expression or initializer for the weights.
 These should be a 5D tensor with shape
- ``(num_input_channels, num_filters, filter_rows, filter_columns)``.
+ ``(num_input_channels, num_filters, input_depth, input_rows,
+- input_columns)``.


Looks like you left an initial - in there.

f0k · 2017-07-31T14:01:33Z

lasagne/layers/conv.py

- def __init__(self, incoming, num_filters, filter_size,
- stride=(1, 1, 1), crop=0, untie_biases=False,
+ def __init__(self, incoming, num_filters, filter_size, stride=(1, 1, 1),
+ crop=0, untie_biases=False,


Unnecessary change, better to avoid, but not a big deal.

Transposed layer after rebase

060d777

frapa mentioned this pull request Jun 17, 2017

Extend TransposedConvLayer #842

Closed

Update docs/conf.py

151ef42

f0k requested changes Jun 21, 2017

View reviewed changes

Fixed documentation references and errors

f496e15

frapa added 3 commits July 4, 2017 18:03

Fixed other duplication and import error

c33b47c

Fixed PEP8 and added tests (to be reviewed)

171abf0

Fixed PEP8 and added tests (to be reviewed)

51dee00

frapa added 3 commits July 4, 2017 18:49

Another fix for tests

5fc53e0

Yet Another fix for tests

0539b64

Yet Another fix for tests (2)

7bc187d

f0k mentioned this pull request Jul 11, 2017

Incorrect He Weight Initialization of Transposed Convolution / Deconvolution Layers #858

Open

f0k requested changes Jul 31, 2017

View reviewed changes

frapa added 6 commits August 19, 2017 16:00

Fixed docstrings (hopefully)

7d38bae

Syncing fork

20e7404

Merge remote-tracking branch 'upstream/master'

5497475

Syncing fork

1ce67b5

Syntax error in test

99e4760

Another import fixed

6793bc4

f0k mentioned this pull request Feb 21, 2018

Support grouped convolutions #900

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transposed Convolution Layers from base class #850

Transposed Convolution Layers from base class #850

frapa commented Jun 17, 2017

frapa commented Jun 17, 2017

f0k left a comment

f0k Jun 21, 2017

f0k Jun 21, 2017

f0k Jun 21, 2017

f0k Jun 21, 2017

f0k Jun 21, 2017

f0k Jun 21, 2017

f0k Jun 21, 2017

f0k Jun 21, 2017

f0k Jun 21, 2017

f0k Jun 21, 2017

f0k commented Jun 21, 2017 •

edited

frapa commented Jul 4, 2017

frapa commented Jul 4, 2017

f0k left a comment

f0k Jul 31, 2017

frapa Aug 19, 2017

f0k Jul 31, 2017

f0k Jul 31, 2017

frapa Aug 19, 2017

f0k Jul 31, 2017

f0k Jul 31, 2017

f0k Jul 31, 2017

f0k Jul 31, 2017

f0k Jul 31, 2017

f0k Jul 31, 2017

f0k Jul 31, 2017

Transposed Convolution Layers from base class #850

Are you sure you want to change the base?

Transposed Convolution Layers from base class #850

Conversation

frapa commented Jun 17, 2017

frapa commented Jun 17, 2017

f0k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f0k commented Jun 21, 2017 • edited

frapa commented Jul 4, 2017

frapa commented Jul 4, 2017

f0k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f0k commented Jun 21, 2017 •

edited