added check for input.key() to be layer instance

[ReyhaneAskari]

Fixes #773. I think I will still need to add tests for coverage.

[ReyhaneAskari]

The failing cases are when the input key type is [None] or <class 'mock.mock.Mock'>. @f0k , what do you suggest to be done in these cases?

[f0k]

what do you suggest to be done in these cases?

The affected tests will need to be changed to play by the rules of get_output and use a Layer instance instead, or a mock that passes the isinstance test. For example: Mock(lasagne.layers.Layer).

[f0k]

That's a good place from an execution standpoint, but not so much for reading the code, since it doesn't match the comment. I'd suggest to move it to the beginning of the function, with a comment saying that it checks whether the input directory is valid, even if this means you'll need an additional if isinstance(inputs, dict) in front. Also KeyError is wrong, that is reserved for missing dictionary keys. It should be a TypeError instead. (See https://docs.python.org/2/library/exceptions.html.)

[ReyhaneAskari]

Done. Thanks.

[ReyhaneAskari]

Thanks @f0k for the explanation. I managed to find a way such that mock would pass the isinstance. But I still need to work on two tests that were using None as the key. I tried to use mock for them too but didn't work. Do you have any suggestions?

[ReyhaneAskari]

My understanding of these two tests is that since we were using None before, the mapping worked but now we also need input_layer or input_layers to make these tests pass.

[f0k]

My understanding of these two tests is that since we were using None before, the mapping worked

You're right. Reading through the tests, it seems this was intentional -- it is currently possible to set the input for a free-floating layer by passing a {None: expression} dictionary to get_output. This is guaranteed by the tests, but not documented. It is also the only supported way to pass some input to a free-floating stack of layers such as: DenseLayer(DenseLayer(DenseLayer((None, 10), 20), 30), 40), because a simple get_output(layer, expression) is forbidden for free-floating layers. We had a long discussion about this back in #52.

Free-floating layers (layers constructed with a shape tuple as the first argument, rather than a Layer instance) were meant as a simple way to use a Lasagne layer as part of a Theano expression, without having to use all of Lasagne. Free-floating stacks of layers were meant as a way to provide the computation to perform in the input-to-hidden or hidden-to-hidden step of a CustomRecurrentLayer, but ultimately we required such stacks to be built on an InputLayer anyway.

We have two options now:

1. Forbid get_output(layer, {None: expression}) and enable get_output(layer, expression) for free-floating layers
2. Ensure get_output{layer, {None: expression}) works as before

The second option is less intrusive. You'd just need to change the beginning of get_output to also allow None as a key, and then not change the tests that use None as a key.

Sorry I forgot about the free-floating layers -- it's a rather obscure use case...

[ReyhaneAskari]

I see. No problem. After changing these tests I did also think that None should have been a supported case, it makes more sense now. I guess I just need to write a coverage test.

[ReyhaneAskari]

I squashed all my commits into one commit.

[f0k]

Thank you, looks good now, including the test! Just a minor problem with the test for None -- I've put a recommendation below. Let me know if I should just update your PR myself.

[f0k]

This is too weak, this would also allow False or '' or 0 as a key. Tests for None should better be explicit. I'd suggest the following (it does some things slightly differently):

# check if the keys of the dictionary are valid
if isinstance(inputs, dict):
    for key in inputs:
        if (key is not None) and (not isinstance(key, Layer)):
            raise TypeError(... % type(key))

[ReyhaneAskari]

Alright. Thanks.

[ReyhaneAskari]

I guess this is good to merge. I am going to work on another ticket.

[f0k]

Yes, thank you, merged this! Was actually trying to merge it yesterday, but github was down.