Skip to content
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

Refactor regularizers and add add_weight method. #4703

Merged
merged 6 commits into from
Dec 14, 2016

Conversation

fchollet
Copy link
Member

@fchollet fchollet commented Dec 14, 2016

This PR does a few things:

  • Refactor regularizers into callable objects, taking a tensor/variable and returning a loss contribution. This is much simpler and more general than the previous setup.
  • Make layers use a add_weight methods to create weights. This removes a lot of boilerplate and complexity.
  • Allows layers to add losses, much like a layer would add updates.

Warning about custom user code:

Custom regularizers may no longer work.
Custom layers may see their regularizers disabled (if they were using regularizers).

We raise appropriate warnings, so these issues should be noticed by the target users.

Additionally, no code would be crashing as a result of these changes.

Feedback welcome.

losses = to_list(losses)
if not hasattr(self, 'losses'):
self.losses = []
try:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The right way to check if a class attribute is a property is:

is_property = isinstance(type(obj).attribute, property)

So here it would be:

if not isinstance(type(self).losses, property):
    self.losses += losses

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fairly sure property is just a wrong word in comment. See for example https://github.com/fchollet/keras/blob/master/keras/layers/core.py#L736

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't actually want to skip the case "is property", because we may well have a property with a getter, in which case appending to list would be the right behavior (e.g. this how trainable_weights currently work).

We explicitly want to skip the case "not settable", distinct from "is property". It isn't clear what is the best way to check if an attribute is settable, by try/except seems like a good candidate: http://stackoverflow.com/questions/6102747/checking-if-property-is-settable-deletable-in-python

# Update self.updates
updates = to_list(updates)
if not hasattr(self, 'updates'):
self.updates = []
try:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too

@kilotaras
Copy link
Contributor

kilotaras commented Dec 14, 2016

It would be better to warn all deprecations at call site. See https://docs.python.org/3/library/warnings.html#warnings.warn for example. Basically just add DepercationWarning, stacklevel=2 to warning.warn calls

Also it would be less painful for everyone involved to add a hard remove version. property is deprecated and will be removed in version x.x.x
Code will be removed in the future making it easier for authors and users won't be surprised when it eventually happens.

@fchollet
Copy link
Member Author

I added an explicit removal date for the deprecated attributes/methods.

@fchollet
Copy link
Member Author

In general, these breaking changes are "fine", because:

  • Only very few users are affected. There are essentially no users who use custom regularizers. There are few users who use custom layers with regularizers.
  • These users are all advanced users, capable of dealing with the problem.
  • These users are explicitly warned during execution of what is happening.
  • We don't break their code, we merely disable auxiliary functionality (regularization).

@fchollet fchollet merged commit ff62eb2 into master Dec 14, 2016
@fchollet fchollet deleted the refactor_regularizers branch December 17, 2016 00:15
@PiranjaF
Copy link
Contributor

PiranjaF commented Feb 5, 2017

If I load the weights of an old model (prior to this PR) with an L2 regularization of X into a new version of Keras with the same L2 regularization of X, then the loss is much higher for the new model than the old on the same dataset (22.0 vs. 3.4). I've tried turning off the L2 regularization, which then makes the losses similar.

@fchollet @farizrahman4u Has the underlying definition of the regularization changed with this implementation or why are the losses suddenly different?

@volvador
Copy link

volvador commented Feb 21, 2017

Using Keras model in another application (distributed tensorflow as in : https://gist.github.com/fchollet/2c9b029f505d94e6b8cd7f8a5e244a4e
I used to do this to deal with regularizers

 model = Sequential()
 ...... #build keras model
 loss = tf.reduce_mean(keras.objectives.mean_squared_error(targets, preds))
 # apply regularizers if any
  if model.regularizers:
    total_loss = loss * 1.  # copy tensor
    for regularizer in model.regularizers:
      total_loss = regularizer(total_loss)
  else:
    total_loss = loss

If I understand well, now I should do

 loss = tf.reduce_mean(keras.objectives.mean_squared_error(targets, preds))
 total_loss = loss * 1.  # copy tensor
 for reg_loss in model.losses:
     tf.assign_add(total_loss, reg_loss)

It crashed when I do that. Any help please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants