New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make lasagne.utils.create_param() get callable that returns Theano expressions or shared variables #695

Merged
merged 1 commit into from Jun 17, 2016

Conversation

Projects
None yet
5 participants
@erfannoury
Copy link
Contributor

erfannoury commented Jun 9, 2016

This issues comments raised in #693

@erfannoury

This comment has been minimized.

Copy link
Contributor Author

erfannoury commented Jun 9, 2016

BTW, I have a question about naming in the PR that isn't yet resolved. What should I do with the name parameter if the callable method returns a shared variable or a Theano expression?

@erfannoury

This comment has been minimized.

Copy link
Contributor Author

erfannoury commented Jun 9, 2016

I have no idea why there are too many failures 😕

@benanne

This comment has been minimized.

Copy link
Member

benanne commented Jun 10, 2016

There seem to be a bunch of failures, and not just in tests for create_param, so that's odd. Have you run the tests locally?

Regarding the name parameter: our assumption is that, when a shared variable is passed in, it may or may not already have been named. If it already has a name, we do not want to overwrite it, so we do nothing. We could technically check if the specified variable has a name, and then insert a name if it doesn't have one, but that would get a bit hairy, I think. Also it wouldn't work for expressions.

@erfannoury

This comment has been minimized.

Copy link
Contributor Author

erfannoury commented Jun 10, 2016

I have only run the test locally for the parts of the code that I have changed. Maybe the failures are affected by the update to the create_param. I will re-check everything now.

Yes, it's written in the comments. My question was specifically for the case that a callable has been passed to the create_param which returns a Theano expression or a shared variables. In this case, should we ignore the name or set it? I think checking for the presence of name is a good solution.

@erfannoury

This comment has been minimized.

Copy link
Contributor Author

erfannoury commented Jun 10, 2016

@benanne One particular thing I encountered when running tests locally vs. Travis CI is that some tests won't pass when Theano is configured to use GPU. What should be done with these test? Some of the tests that previously were in the repository (not added by this PR) are prone to this problem, too.

@erfannoury

This comment has been minimized.

Copy link
Contributor Author

erfannoury commented Jun 11, 2016

@benanne I wonder how these two tests passed before:

  • lasagne/tests/layers/test_special.py::TestParametricRectifierLayer::test_alpha_init
  • lasagne/tests/layers/test_special.py::TestParametricRectifierLayer::test_get_output_for

In both tests, a scalar is passed to create_param, however I think (and I even ran the abovementioned tests using the original source code) these two tests should fail anyway. What am I missing here?

Now the build failures are down to these two and a doctest error on the EmbeddingLayer (lasagne/layers/embedding.py: lasagne.layers.embedding.EmbeddingLayer)

@f0k

This comment has been minimized.

Copy link
Member

f0k commented Jun 14, 2016

What am I missing here?

You changed the behaviour of create_param(). Before, when it got a callable, it called the callable and converted the result with lasagne.utils.floatX. Now you call the callable and run the result through the if cases, which don't grip for Python scalars. You'll have to keep your modifications to the original third if case or change the function to also accept Python scalars. Something like this:

if spec is callable:
    spec = spec(shape)
if spec is Python scalar:
    spec = floatX(spec)
if spec is numpy array:
    check for correct shape
    spec = theano.shared(spec)
if spec is theano variable:
    check for correct dimensionality
    add name if unset
else:
    complain that the spec is unsupported

Note that the if cases are not elif cases.

Instead of setting a boolean flag to spit out the correct error message in the end, you could gradually build up a prefix string for the error message inside the if cases ("the numpy array", "the Theano variable", "the numpy array returned by the callable", ...).

@erfannoury

This comment has been minimized.

Copy link
Contributor Author

erfannoury commented Jun 14, 2016

Ok, @f0k thanks for the suggestions. All tests pass now except the EmbeddingLayer's doctest, which I'm looking into now. I'll try updating the way error messages are handled, too.

@erfannoury

This comment has been minimized.

Copy link
Contributor Author

erfannoury commented Jun 14, 2016

I can't reproduce the doctest failure in EmbeddingLayer in either CPU or GPU mode on my computer.

spec = spec(shape)
callable_prefix = "cannot initialize parameters: the provided " \
"callable did not return a value with the " \
"correct shape or dimensions."

This comment has been minimized.

@f0k

f0k Jun 15, 2016

Member

Just to make the errors a little nicer:

err_prefix = "cannot initialize parameter %s: " % name
if callable(spec):
    spec = spec(shape)
    err_prefix += "the %s returned by the provided callable"
else:
    err_prefix += "the provided %s"
...
if isinstance(spec, np.ndarray):
    if spec.shape != shape:
        raise ValueError("%s has shape %s, should be %s") % (err_prefix % "numpy array", spec.shape, shape)
    ...

if isinstance(spec, theano.Variable):
    if spec.ndim != len(shape):
        raise ValueError("%s has %d dimensions, should be %d" % (err_prefix % "Theano variable", spec.ndim, len(shape))
else:
    if "callable" in err_prefix:
        raise TypeError("%s is not a numpy array or a Theano expression" % (err_prefix % "value"))
    else:
        raise TypeError("%s is not a numpy array, a Theano expression, or a callable" % (err_prefix % "value"))

Note that I've changed RuntimeError to ValueError or TypeError (we need to clean this up elsewhere as well), the tests need to change accordingly.

@erfannoury

This comment has been minimized.

Copy link
Contributor Author

erfannoury commented Jun 15, 2016

Changed the error handling to what @f0k suggested. Tests are also updated accordingly and all pass. Only the EmbeddingLayer's doctest fails, however I can't reproduce it locally.

"""
shape = tuple(shape) # convert to tuple if needed
if any(d <= 0 for d in shape):
raise ValueError((
"Cannot create param with a non-positive shape dimension. "
"Tried to create param with shape=%r, name=%r") % (shape, name))

err_prefix = "cannot initialize parameter %s: " % name
if hasattr(spec, '__call__'):

This comment has been minimized.

@f0k

f0k Jun 15, 2016

Member

I'd prefer if callable(spec).

This comment has been minimized.

@erfannoury

erfannoury Jun 15, 2016

Author Contributor

Ok. I'll change this, too.

raise RuntimeError("cannot initialize parameters: 'spec' is not "
"a numpy array, a Theano expression, or a "
"callable")
if hasattr(spec, '__call__'):

This comment has been minimized.

@f0k

f0k Jun 15, 2016

Member

This is not going to work, spec has been replaced by then. As in my proposal, it should be if "callable" in err_prefix. It's a bit ugly, but otherwise the error message is less precise.

This comment has been minimized.

@erfannoury

erfannoury Jun 15, 2016

Author Contributor

Yes you're right. I've messed it up here.



def unroll_scan(fn, sequences, outputs_info, non_sequences, n_steps,
go_backwards=False):
"""

This comment has been minimized.

@f0k

f0k Jun 15, 2016

Member

Interesting that nobody noticed this before. But anyway, this shouldn't be part of your PR! Please leave the indentation for unroll_scan as it was.

"callable")
if hasattr(spec, '__call__'):
raise TypeError("%s is not a numpy array or a Theano expression" %
(err_prefix % "value"))

This comment has been minimized.

@f0k

f0k Jun 15, 2016

Member

This exception is missing in the tests now. We'll need a test_create_param_callable_returns_wrong with a callable that returns a string, for example.

@f0k

This comment has been minimized.

Copy link
Member

f0k commented Jun 15, 2016

I can't reproduce the doctest failure in EmbeddingLayer in either CPU or GPU mode on my computer.

This is still a mystery. Does the new create_param change the dtype of a plain numpy array (i.e., is it passed through floatX)? Did you try running the test locally with THEANO_FLAGS=device=cpu,floatX=float64?

err_prefix += "the provided %s"

if np.isscalar(spec):
spec = floatX(spec)

This comment has been minimized.

@f0k

f0k Jun 15, 2016

Member

np.isscalar("asdf") returns True. We need to explicitly check for numbers.

if spec.shape != shape:
raise ValueError("%s has shape %s, should be %s" %
(err_prefix % "numpy array", spec.shape, shape))
spec = theano.shared(floatX(spec))

This comment has been minimized.

@f0k

f0k Jun 15, 2016

Member

Ah, you sneaked in a floatX here. This wasn't present before in the codepath for an ndarray. That's why the doctest for the EmbeddingLayer fails. We should have a more direct test that checks whether the dtype of an ndarray is retained in create_param!

This comment has been minimized.

@f0k

f0k Jun 15, 2016

Member

Checking the original code again, there is a conversion to floatX for values returned by a callable, but not for values directly passed as a numpy array. This probably wasn't originally intended, but I don't know if any code relies on this. As far as I see, all initializers provided by Lasagne produce values of the correct dtype, but third-party code might not.

This comment has been minimized.

@erfannoury

erfannoury Jun 15, 2016

Author Contributor

So if the code has to retain the original type of the input ndarray, then the code should change a bit. I'll add a test to be sure to retain the type in this case.

@erfannoury

This comment has been minimized.

Copy link
Contributor Author

erfannoury commented Jun 15, 2016

All tests are passing now. @f0k could you please review the code once more?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 15, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling cd65a2a on erfannoury:th-expr-param into 8fe645d on Lasagne:master.

err_prefix += "the provided %s"

if isinstance(spec, numbers.Number):
spec = np.asarray(spec)

This comment has been minimized.

@f0k

f0k Jun 16, 2016

Member

Nice find! This seems slightly too strict, though (I'm sorry that this PR turns out to be much more intricate than it seemed at first glance):

>>> x = np.float32(123)
>>> isinstance(x, numbers.Number)
False

Maybe:

if isinstance(spec, numbers.Number) or isinstance(spec, np.generic) and spec.dtype.kind in 'biufc':

Although this is getting silly. Let's leave it at what you have.

This comment has been minimized.

@erfannoury

erfannoury Jun 16, 2016

Author Contributor

(I'm sorry that this PR turns out to be much more intricate than it seemed at first glance)

No, it's ok. Sorry that I have been slow in implementing the correct code. This is my first PR for Lasagne 😄

>>> x = np.float32(123)
>>> isinstance(spec, numbers.Number)
False

@f0k I think this code isn't completely correct. numbers.Number in fact correctly handles the case you have provided.

>>> x = np.float32(123)
>>> isinstance(x, numbers.Number)
True

As far as I tested multiple cases, numbers.Number correctly handles scalars of multiple types and kinds.

This comment has been minimized.

@f0k

f0k Jun 16, 2016

Member

I think this code isn't completely correct. numbers.Number in fact correctly handles the case you have provided.

Interesting. Not for me on Python 2.7.6 or 3.4.2, numpy 1.8.2.

This comment has been minimized.

@erfannoury

erfannoury Jun 16, 2016

Author Contributor

You've passed spec as argument to isinstance, instead of x. I'm working on Python 2.7.11, NumPy 1.11.0.

This comment has been minimized.

@f0k

f0k Jun 16, 2016

Member

No, I had just written it wrongly, I passed x. It depends on the numpy/Python version than. In any case, it indicates we don't need to do such a stretch here and can keep it as you have it.

This comment has been minimized.

@erfannoury

erfannoury Jun 16, 2016

Author Contributor

But it won't work correctly in those combinations of NumPy/Python.
So I should keep them as is?

This comment has been minimized.

@f0k

f0k Jun 16, 2016

Member

But it won't work correctly in those combinations of NumPy/Python.

It's a corner case. Using scalars was not actually documented, it just happened to be used in one of the tests. Using numpy scalars instead of Python scalars is even more undocumented.

So I should keep them as is?

You can also add my modified if statement, as you please. Either way is fine by me!

This comment has been minimized.

@erfannoury

erfannoury Jun 16, 2016

Author Contributor

Yes, I think that's a more robust solution. I'll update the if statement.

raise ValueError("%s has shape %s, should be %s" %
(err_prefix % "numpy array", spec.shape, shape))
if "callable" in err_prefix:
spec = floatX(spec)

This comment has been minimized.

@f0k

f0k Jun 16, 2016

Member

Yes, this restores original behaviour. I'm leaning towards breaking compatibility, though, and announcing it on the mailing list.
@benanne: Would you be fine with not converting results of callables to floatX? We never did this for numpy arrays passed as initial values, just for numpy arrays returned by callables passed as initial values. This is inconsistent.

This comment has been minimized.

@erfannoury

erfannoury Jun 16, 2016

Author Contributor

Ok, I'll wait for the final decision.

This comment has been minimized.

@benanne

benanne Jun 16, 2016

Member

Yeah, I think it would be fine to change this.

This comment has been minimized.

@erfannoury

erfannoury Jun 17, 2016

Author Contributor

@f0k done.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 16, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling d50bd94 on erfannoury:th-expr-param into 8fe645d on Lasagne:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 16, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 1955df9 on erfannoury:th-expr-param into 8fe645d on Lasagne:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 17, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 169d6f7 on erfannoury:th-expr-param into 8fe645d on Lasagne:master.

Either of the following:
* a numpy array with the initial parameter values
* an scalar or a numpy array with the initial parameter values

This comment has been minimized.

@f0k

f0k Jun 17, 2016

Member

*a scalar (word starts with consonant)

@@ -305,30 +306,50 @@ def create_param(spec, shape, name=None):
name : string, optional
If a new variable is created, the name to give to the parameter
variable. This is ignored if `spec` is already a Theano expression
or shared variable.
or shared variable and which already has a name.

This comment has been minimized.

@f0k

f0k Jun 17, 2016

Member

Needs to be changed a bit because it doesn't only apply when a new variable is created now:

The name to give to the parameter variable. Ignored if `spec` is or returns a Theano expression or shared variable that already has a name.
contain this array. If a shared variable or expression was provided,
it is simply returned. If a callable was provided, it is called, and
its output is used to initialize a shared variable.
If an scalar or a numpy array was provided, a shared variable is

This comment has been minimized.

@f0k

f0k Jun 17, 2016

Member

*a scalar

"""
# to check if argument is a number
import numbers

This comment has been minimized.

@f0k

f0k Jun 17, 2016

Member

If you're fixing the spelling above, could you make this a line comment?

import numbers  # to check if argument is a scalar number

Otherwise it looks as if this comment applies to the whole following code block.

@f0k

This comment has been minimized.

Copy link
Member

f0k commented Jun 17, 2016

Very cool! Just three minor requests. In the end, could you please squash this into a single commit? It should go like this:

git reset d5fbd64  # set HEAD to the first commit of this PR
git commit -a --amend -m 'Rewrite create_param() to allow callables returning Theano variables'  # amend all other changes to that commit
git push --force origin HEAD  # overwrite PR with the new clean history
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 17, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 776ce5f on erfannoury:th-expr-param into 1565826 on Lasagne:master.

@erfannoury

This comment has been minimized.

Copy link
Contributor Author

erfannoury commented Jun 17, 2016

@f0k squashed as you said, and all tests are passing.

@f0k

This comment has been minimized.

Copy link
Member

f0k commented Jun 17, 2016

Great, thanks a lot, everything looks fine! Will merge in the evening if there are no further comments.

@f0k f0k merged commit b2f42fd into Lasagne:master Jun 17, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@f0k

This comment has been minimized.

Copy link
Member

f0k commented Jun 17, 2016

Merged, thanks again!

@twiecki

This comment has been minimized.

Copy link

twiecki commented Jun 17, 2016

👍

@benanne benanne referenced this pull request Aug 8, 2016

Closed

Bayes by Backprop #731

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment