get_output fails when input is a np.ndarray

[lasagne]

get_output fails when input is a np.ndarray

[benanne]

It's a change in only one place, so I'm for it. Since get_output_for can take an np.ndarray just fine, it should also be allowed for get_output.

Nevertheless I'd like @f0k to weigh in on this as well, he's really good at spotting edge cases and possible issues!

[f0k]

Hmm, interesting, I didn't know Theano allows this! So one thing we could do would be to change the InputLayer method to:

def get_output(self, input=None, *args, **kwargs):
    if input is None:
        return self.input_var
    elif isinstance(input, dict):
        # (extra change: fall back to input_var if not in dict)
        return input.get(self, self.input_var)
    else:
        return input

But then we need to ensure each get_output_for implementation only uses methods on input that are available both for Theano expressions and numpy arrays -- e.g., shape, ndim and flatten are fine, dimshuffle is not. An alternative would be to explicitly wrap numpy arrays in InputLayer.get_output in Theano constants, because that's what Theano does internally:

In [1]: import theano
Using gpu device 0: GeForce GTX 780 Ti

In [2]: a = theano.tensor.matrix()

In [3]: b = numpy.zeros((2,2))

In [4]: theano.printing.debugprint(a+b)
Elemwise{add,no_inplace} [@A] ''   
 |<TensorType(float32, matrix)> [@B]
 |TensorConstant{(2, 2) of 0.0} [@C]

This way get_output_for can be sure it always deals with Theano expressions. But maybe we also want get_output_for to work for numpy arrays, not only get_output? In this case any get_output_for that wants to do things like dimshuffles needs to check for inputs being numpy arrays and wrap them in Theano expressions or have two code paths. Probably there are not too many implementations that need to distinguish the two cases.

In any case, get_output and get_output_for need proper docstrings for what they support!

[benanne]

But then we need to ensure each get_output_for implementation only uses methods on input that are available both for Theano expressions and numpy arrays

I think this would be requiring too much of the user. We want get_output_for to be easy to implement, because that's the first thing you need to do to implement a custom Layer subclass (an extremely important use case). So I think it would be best to guarantee that its input is always a valid Theano expression. If Theano's and numpy's interfaces were more consistent with each other this would not be such a big issue, but unfortunately there are things like dimshuffle and they are used very frequently.

Then the question becomes, do we want to support this with get_output (i.e. should we do the wrapping ourselves as in @f0k's second proposal), or do we assume that the user takes care of this and make it clear that both get_output and get_output_for take only Theano expressions as input.

Clearly there is a use case for having get_output support this, since @craffel just found one :) But maybe we would want to keep the behaviour of get_output and get_output_for as similar as possible.

[craffel]

I think either of the solutions are fine, i.e. only allow theano variables as input, or when the input is not a theano variable try casting it as a tensor constant and throw a helpful error if it fails.

[f0k]

There are other cases where we allow both Theano expressions and non-Theano-expressions, such as the learning rate in nntools.updates.*. That could be an argument for supporting numpy arrays and Python scalars here as well. The only counter-argument I see is that this might lead users into doing silly things without realising what's happening behind the scenes (altough I cannot think of a good example, maybe there's not much that can go wrong).

Anyway, if we go for supporting numpy arrays, I'd vote for:

def get_output(self, input=None, *args, **kwargs):
    if isinstance(input, dict):
        input = dict.get(self, None)
    if input is None:
        return self.input_var
    elif isinstance(input, theano.gof.Variable):
        return input
    else:
        try:
            return theano.tensor.constant(input)
        except Exception as e:
            raise TypeError("Input of type %s is not a Theano expression and cannot be wrapped as a Theano constant (original exception: %s)" % (type(input), e))

[craffel]

Your code snippet is what I had in mind, sounds good to me. I also can't think of a good example for using it incorrectly either.

[f0k]

@benanne: I think this issue can be closed.

[benanne]

Whoops, you're right. Thanks for the reminder!