Implement MovingAverage optimizer #215

Squadrick · 2019-04-27T18:45:01Z

Closes #5

Squadrick · 2019-04-27T18:50:19Z

@seanpmorgan @facaiy Sorry about the delay on this. Could you take a look at this PR?

Also, I've made a few changes to the API.

In contrib, we had to use swapping_saver() to get an object of tf.train.Saver, to get the running mean and save those weights.
In addons, I've made assign_average_vars(var_list) that assigns the running mean to the variables in var_list.

I've included a docstring with an example that should show the new API usage. Let me know what you think.

* Port MovingAverageOptimizer from tf.contrib.opt * Inherits base Keras optimizer_v2 * `swapping_saver` replaced with `assign_average_vars` * Update test cases for TF2.X * Update docs

tensorflow_addons/optimizers/moving_average.py

tensorflow_addons/optimizers/README.md

Smokrow · 2019-04-28T14:00:03Z

Thank you for your PR :). I left a few reviews above

Squadrick · 2019-04-28T20:34:22Z

@Smokrow Made a change regarding the internal function, and I've left comments regarding the colab notebook example and paper. Thanks for taking the time to review this PR.

facaiy · 2019-04-29T03:30:28Z

@Smokrow Good job, Moritz, thanks for your help :-)

@WindQAQ Hi, Tzu-Wei, do you have time to take an another look? I'm kind of busy this week.

WindQAQ

LSTM! Thanks for the contribution! Could you make the tests more compatible with eager modes? And other minor requests are in the comments

tensorflow_addons/optimizers/moving_average_test.py

tensorflow_addons/optimizers/moving_average.py

tensorflow_addons/optimizers/moving_average_test.py

tensorflow_addons/optimizers/moving_average.py

WindQAQ · 2019-04-29T07:21:24Z

Also, do we really need to override _create_slots? Seems that slots will be created while calling self._optimizer.apply_gradient in apply_gradients. And I didn't see this method used in other places.
https://github.com/tensorflow/tensorflow/blob/master/tensorflow/python/keras/optimizer_v2/optimizer_v2.py#L427

* Use _set_hyper() and _get_hyper() instead of member variables for average_decay, num_updates and sequential_update * Remove _create_slots() from MovingAverage * Use _serialize_hyperparameter() in get_config() * Replace if-else with tf.cond() to work with tensors * Use absolute import of tensorflow_addons in moving_average_test.py

WindQAQ

Hi @Squadrick, I'm not aware that using _set_hyper for bool and None type will result in lots of works. Could you revert sequential_update and num_updates to the original implementation? Sorry for any convenience caused.

* Tests modified for static and eager execution * num_updates and sequential_update reverted back to instance variables * Type check of num_updates and sequential_update

Squadrick · 2019-04-29T14:29:54Z

@WindQAQ Reverted them back and added support for eager execution along with the tests.

WindQAQ

Very close! Seems that some test cases fail? And also make sure to run make code-format before committing. Thanks for the contribution :-)

tensorflow_addons/optimizers/moving_average.py

* Remove six import in moving_average_test * Wrap zip objects in list to pass tests in python3 * Fix typos

Squadrick · 2019-04-29T15:41:30Z

@WindQAQ Had to wrap all zip objects in list() to get the tests to pass in python3, didn't know about that. Everything should be working now, could you trigger the CI?

WindQAQ · 2019-04-29T15:54:35Z

Had to wrap all zip objects in list() to get the tests to pass in python3, didn't know about that.

It's likely to be the difference caused by generator. Anyway, thanks for the contribution! I'm looking forward to seeing the example colab notebook :-)

Squadrick · 2019-04-29T15:56:49Z

@WindQAQ @Smokrow Thanks for the reviews, guys.

guillaumekln · 2019-09-23T12:56:06Z

tensorflow_addons/optimizers/moving_average.py

+
+        with tf.name_scope(name):
+            self._ema = tf.train.ExponentialMovingAverage(
+                average_decay, num_updates=num_updates)


Shouldn't the constructor pass optimizer.iterations as num_updates?

guillaumekln · 2019-09-23T13:17:00Z

tensorflow_addons/optimizers/moving_average.py

+        base_config = self._optimizer.get_config()
+        return dict(list(base_config.items()) + list(config.items()))
+
+    def assign_average_vars(self, var_list):


Checkpoints are usually saved in regular intervals. Is it a standard practice to continue training with the averaged variables after saving a checkpoint?

Squadrick requested review from facaiy and WindQAQ as code owners April 27, 2019 18:45

googlebot added the cla: yes label Apr 27, 2019

Squadrick added 2 commits April 28, 2019 16:26

Implement MovingAverage optimizer

47c9654

* Port MovingAverageOptimizer from tf.contrib.opt * Inherits base Keras optimizer_v2 * `swapping_saver` replaced with `assign_average_vars` * Update test cases for TF2.X * Update docs

Add moving_average_test as a py_test in BUILD file

25d33f4

Smokrow suggested changes Apr 28, 2019

View reviewed changes

Move internal functions under external functions to improve readability

d2420c9

Squadrick force-pushed the moving-avg-opt branch from b47ae08 to d2420c9 Compare April 28, 2019 20:32

WindQAQ requested changes Apr 29, 2019

View reviewed changes

WindQAQ self-assigned this Apr 29, 2019

WindQAQ added the optimizers label Apr 29, 2019

WindQAQ self-requested a review April 29, 2019 13:26

WindQAQ requested changes Apr 29, 2019

View reviewed changes

Add eager execution support to MovingAverage

ba51cb5

* Tests modified for static and eager execution * num_updates and sequential_update reverted back to instance variables * Type check of num_updates and sequential_update

WindQAQ added the kokoro:force-run label Apr 29, 2019

kokoro-team removed the kokoro:force-run label Apr 29, 2019

WindQAQ self-requested a review April 29, 2019 14:47

WindQAQ requested changes Apr 29, 2019

View reviewed changes

tensorflow_addons/optimizers/moving_average.py Outdated Show resolved Hide resolved

tensorflow_addons/optimizers/moving_average.py Outdated Show resolved Hide resolved

Nit fixes

30b12ee

* Remove six import in moving_average_test * Wrap zip objects in list to pass tests in python3 * Fix typos

WindQAQ added the kokoro:force-run label Apr 29, 2019

kokoro-team removed the kokoro:force-run label Apr 29, 2019

WindQAQ approved these changes Apr 29, 2019

View reviewed changes

WindQAQ merged commit e278852 into tensorflow:master Apr 29, 2019

Squadrick deleted the moving-avg-opt branch April 29, 2019 15:56

guillaumekln reviewed Sep 23, 2019

View reviewed changes

Implement MovingAverage optimizer #215

Implement MovingAverage optimizer #215

Uh oh!

Conversation

Squadrick commented Apr 27, 2019

Uh oh!

Squadrick commented Apr 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Smokrow commented Apr 28, 2019

Uh oh!

Squadrick commented Apr 28, 2019

Uh oh!

facaiy commented Apr 29, 2019

Uh oh!

WindQAQ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WindQAQ commented Apr 29, 2019

Uh oh!

WindQAQ left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Squadrick commented Apr 29, 2019

Uh oh!

WindQAQ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Squadrick commented Apr 29, 2019

Uh oh!

WindQAQ commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Squadrick commented Apr 29, 2019

Uh oh!

guillaumekln Sep 23, 2019

Choose a reason for hiding this comment

Uh oh!

guillaumekln Sep 23, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Squadrick commented Apr 27, 2019 •

edited

Loading

WindQAQ left a comment •

edited

Loading

WindQAQ commented Apr 29, 2019 •

edited

Loading