Shape error message

[Erotemic]

This is a small change, I'm not sure if there are guidelines for contributions and how involved they should be. If small changes like this are ok, then I'll continue to make pull requests as I modify the library. If larger changes are preferred I'll hold off until I feel I've made a more substantial progress.

I added an error message to catch the scenario where the conv+max pooling layers reduce an image size to 0 for smaller images.

Previous error messages looked like this, and had a delay due to the SVD call during orthogonal initialization.

  ...
  File "/home/joncrall/code/ibeis_cnn/ibeis_cnn/models.py", line 1012, in build_model
    W=init.Orthogonal(),
  File "/home/joncrall/code/Lasagne/lasagne/layers/dense.py", line 72, in __init__
    self.W = self.add_param(W, (num_inputs, num_units), name="W")
  File "/home/joncrall/code/Lasagne/lasagne/layers/base.py", line 228, in add_param
    param = utils.create_param(spec, shape, name)
  File "/home/joncrall/code/Lasagne/lasagne/utils.py", line 271, in create_param
    arr = spec(shape)
  File "/home/joncrall/code/Lasagne/lasagne/init.py", line 29, in __call__
    return self.sample(shape)
  File "/home/joncrall/code/Lasagne/lasagne/init.py", line 354, in sample
    u, _, v = np.linalg.svd(a, full_matrices=False)
  File "/usr/local/lib/python2.7/dist-packages/numpy/linalg/linalg.py", line 1306, in svd
    _assertNoEmpty2d(a)
  File "/usr/local/lib/python2.7/dist-packages/numpy/linalg/linalg.py", line 222, in _assertNoEmpty2d
    raise LinAlgError("Arrays cannot be empty")
LinAlgError: Arrays cannot be empty

They the error messages are shorter and there is no delay.

  ...
  File "/home/joncrall/code/ibeis_cnn/ibeis_cnn/models.py", line 1142, in build_model
    W=init.Orthogonal(),
  File "lasagne/layers/dense.py", line 72, in __init__
    self.W = self.add_param(W, (num_inputs, num_units), name="W")
  File "lasagne/layers/base.py", line 229, in add_param
    param = utils.create_param(spec, shape, name)
  File "lasagne/utils.py", line 253, in create_param
    raise RuntimeError('An element in param shape is 0. shape=%r, name=%r' % (shape, name))
RuntimeError: An element in param shape is 0. shape=(0, 512), name='W'

[ebenolson]

👍 to more friendly error messages. I think ValueError is probably the most appropriate here.

[f0k]

You can omit the square brackets to make it a generator expression instead of a list comprehension.
/edit: And if 0 in shape would be even more concise. But maybe we should change it to d < 0 for d in shape to catch more problems.

[benanne]

Small changes are definitely okay! We just have to make sure that all the small changes add up to something consistent and coherent over time :)

It seems that the original error message you quote would only come up with the lasagne.init.Orthogonal initializer. Did you also test what happens with other initializers (the default one for example)? Are the error messages similarly vague there? Or maybe it doesn't even raise anything at all in that case?

I would formulate the error message slightly differently. Currently it only states the fact that a shape tuple has a zero in it. Something more informative would be "all shape elements should be > 0, but received (...)", I think. Maybe we need a better name for "shape element" as well. Is there any convention for this used by numpy / Theano?

Also make sure to follow the Python PEP8 style guide, which we try to adhere to. Otherwise the Travis build will fail because it runs these checks. In this case, the line that raises the error is too long (lines should be 79 characters max). You can verify this by running our test suite, which will also run the PEP8 checks. Have a look at the development docs for more info: http://lasagne.readthedocs.org/en/latest/user/development.html

[f0k]

That line is too long for pep8, please break it apart a bit. Plus, as Eben said, a ValueError would be more appropriate. We need to change all those RuntimeErrors into something more suitable (#196)!
Finally, I'd slightly change the error message to: "Tried to create param %r with shape containing zeros: %r"
This hints at the fact that the creation of a parameter failed (as opposed to an existing parameter being found with a funny shape).

[Erotemic]

I've edited the commit based on comments.

I went back and tested on non-Orthogonal initialization. It turns out that it fails silently if the shape becomes 0 or negative if the default initialization is used. I believe this is an error. To fix it I added a similar check on the lasagne.layers.base.Layer.init function.

When running the tests I'm getting several failures because the Mock object is not iterable.
The exact error is TypeError: 'Mock' object is not iterable.
I'm unfamiliar with Mock, and I immediately don't the best approach for rectifying this error.

[benanne]

Do we need both additions though? I guess one is for the parameter shapes, and one is for the layer input shapes, but maybe they both handle the same issues.

One thing to consider is whether we want a layer to complain about its input shape like this - maybe the layer before it should instead be guaranteed to have a correct output shape. It's not entirely clear to me where we should put this responsibility. I guess ensuring correct input shapes is much easier than ensuring correct output shapes, because we can do it in the base class.

Regarding the errors: mock objects are used in tests to replace 'real' objects and to be able to check how they are used by the code (which fields are accessed, which methods were called on them, etc.). Unfortunately they are not iterable, and your addition tries to iterate over Layer.input_shape. Since this will always be an iterable in practice, it looks like we will need to update the tests.

If you don't think you can do this, I would suggest leaving the second error message out of this PR for now, we can always add that later. Alternatively someone could create a PR to your PR, but I don't know if anyone will be up for that right now.

EDIT: also, while I'm in favour of defensive programming and good error messages, I'm a bit wary of sprinkling all these if statements around the code - there may be a point where it starts hampering code readability. We should take that into consideration as well.

[Erotemic]

I'm also unsure if it needs to be put in both add_param and Layer.init. I was also unsure of the who's responsibility it was to raise the error. I think that it is necessary to have the error somewhere. Its very easy to create a convolutional network with too small of an input layer. That error should be caught when the layers are connected or created.

The other option for where the error message could go is the output_shape property. It could check the output shape before it is returned. I don't think it should go in get_output_shape_for because that will be overridden and the error message should not have to be re-implemented. The problem with putting it in the output_shape property is that no checks will occur if get_output_shape_for is called. On the other hand, if the check is on Layer creation, then it is guaranteed to be called, and called once. However, I do agree that it should not be the a Layer's responsibility to check the integrity of its input.

An alternative is that on Layer.init it checks its own output_shape property. This is slightly different than the current state of the commit because the Layer checks its own output, once, on creation. I think this is the correct solution.

I think adding a param and creating a Layer are enough different that the error should exist in both places. However, I've only been programming with Lasagne for a few weeks, so I could be wrong here.

Lastly, in terms of code readability, I'm all for keeping that as a primary consideration. I'm a buyer of the Tim Peters philosophy. However, without these messages, the error was only caught by a call to np.linalg.svd, and it was silent when Orthogonal initialization was not being used. Readability counts, but errors should never pass silently (or be caught by a cpu intensive singular value decomposition).

[f0k]

I think it should be checked both in Layer.__init__ and in create_param() because they do different things. They are not independent, of course, because the shape for the parameters usually depends on the input shape, but it would be good to catch the error as early as possible. We still cannot completely omit the check in create_param(), because there could be other reasons for an invalid parameter shape, such as num_units <= 0 for a DenseLayer.
Regarding checking the output shape: That would be useful because it would also check the output shape of the very last layer in a network, but we cannot guarantee that the output_shape property (or the get_output_shape_for() function) will be called at all.

An alternative is that on Layer.init it checks its own output_shape property.

That's not possible, unfortunately: When the base class constructor is called, the subclass usually has not finished (or even begun) initializing its attributes, so get_output_shape_for() doesn't work. Otherwise, we would have made output_shape an attribute instead of a property (see the discussion around here: #182 (comment)). We would have to require every subclass to call a post-constructor function from the base class that initializes and checks the output shape. This would be useful from a basic user's perspective (output_shape could become an attribute instead of a property, and any kind of mistake preventing the shape computation would be evident on constructing the layer, not only when adding a layer on top), but bad from an advanced user's perspective (another thing to learn about and remember when writing your own layer class).

When running the tests I'm getting several failures because the Mock object is not iterable.

One way would be replacing Layer(Mock()) by Layer(Mock(output_shape=(None,)). This will give the Mock that serves as the layer's incoming layer an output_shape attribute of (None,).

[Erotemic]

I was able to fix the failing tests. The coverage went down because there are currently no test cases specifically triggering the errors.

[benanne]

Nice, thanks! It would be great to have those tests though, so we know they are triggered only in the right occasions and not otherwise :)

EDIT: as an example, I just added a bunch of tests checking for exceptions for the convolution layers, maybe those are handy to use as inspiration: https://github.com/Lasagne/Lasagne/blob/master/lasagne/tests/layers/test_conv.py#L329

[f0k]

Wow, coveralls even marks this as a failure. I think I like that. It really urges us to include tests with all our PRs. Sorry if it makes contributions harder even for seemingly simple changes, but we've benefitted from our tests a couple times and you will learn to love them as well when you contribute more :) Don't hesitate to ask us if you need some more guidance!

[Erotemic]

I added tests which brought the coverage back up.

[benanne]

Looks good to me :) It's a bit unfortunate that we have to have Mock(output_shape=(None,)) everywhere now, which makes the test code a bit harder to read. But I can't really think of a better solution either.

Other than that it would be nice to squash the commits for this PR, there are quite a few corrections and merge commits. If you don't know how to do that I will defer to @f0k for an explanation, because I don't know off the top of my head either :p

[f0k]

Please don't do random code style changes as part of a PR. I know some editors are set to do that, but please don't commit such changes. The previous version was better anyway, because the extra indentation level makes clear it's not the next level of control.

[Erotemic]

I changed this because I got a pep8 failure when running py.test

/home/joncrall/code/Lasagne/lasagne/tests/layers/test_normalization.py:53:25: E126 continuation line over-indented for hanging indent
                        row=c01b[:, r, c, x],

I'll try undoing that and committing. Maybe Travis CI doesn't have that check enabled.

[benanne]

I have actually seen the same PEP8 error on some machines but not others. It doesn't occur on my laptop for example, where I do most of my development (nor on Travis). It might be related to what version of pep8 is installed.

[f0k]

Ah, I remember, you had the same problem while you were here, right? Shall we add --ignore=E126 to our configuration so we can have hanging indents with 8 spaces, or shall we generally change all hanging indents to 4 spaces?

Sorry for blaming you, Jon, in the past we had various whitespace/linebreak changes in commits that were just a matter of taste rather than a matter of pep8 compliance. We should put up a general "commit etiquette" for Lasagne somewhere.

[f0k]

Thanks for the tests! So that "small change" required a lot more changes than we thought...

It's a bit unfortunate that we have to have Mock(output_shape=(None,)) everywhere now, which makes the test code a bit harder to read.

The Mocks make the test code harder to read anyway -- it worked fine in the beginning when we didn't check any attributes in the constructor and didn't have any isinstance or hasattr checks in the output propagation, but now we need the Mocks to behave much more like real layers. We might consider having some global test fixtures that help creating proper mocks for our purposes. (After this PR.)

[Erotemic]

I rebased and squashed all of the commits into a single commit (first time I've done this, but I think it went ok).

The extra spacing is still in there. I don't have any particular opinion on hanging indentation. It might be easier to just add the pep8 ignore. Its odd that some machines fail on that and other's don't. These are the versions of the python linters on my machine: flake8 2.2.5 (pep8: 1.5.7, mccabe: 0.2.1, pyflakes: 0.8.1) CPython 2.7.6 on Linux

[benanne]

Here are the version numbers on my laptop (no failure):
flake8 2.3.0
pep8 1.6.1
mccabe 0.3
pyflakes 0.8.1

and on the machine which does show the error:
flake8 2.4.0
pep8 1.5.7
mccabe 0.3
pyflakes 0.8.1

Looks like it might be related to which pep8 version is installed. 1.5.7 fails on those lines, 1.6.1 doesn't.

[dnouri]

Duh. I guess this means we need an extra requirements-dev.txt for the stuff that's installed when you run python setup.py dev, i.e. tests_require.

[benanne]

Looks like this one is also ready for merging, sorry for the wait!