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

AdaBound + AMSBound #137

Merged
merged 14 commits into from Dec 24, 2019
Merged

AdaBound + AMSBound #137

merged 14 commits into from Dec 24, 2019

Conversation

@zoq
Copy link
Member

zoq commented Sep 21, 2019

Implementation of AdaBound and AMSBound which employ dynamic bounds on
learning rates to achieve a gradual and smooth transition from adaptive methods to
SGD. For more information see "Adaptive Gradient Methods with Dynamic Bound of Learning Rate" by Luo, Liangchen and Xiong, Yuanhao and Liu, Yan and Sun, Xu.

zoq added 2 commits Sep 21, 2019
zoq added 3 commits Sep 21, 2019
@zoq

This comment has been minimized.

Copy link
Member Author

zoq commented Sep 25, 2019

@mlpack-jenkins test this please

@zoq zoq added the s: needs review label Sep 25, 2019
@mlpack-bot

This comment has been minimized.

Copy link

mlpack-bot bot commented Oct 25, 2019

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! 👍

@mlpack-bot mlpack-bot bot added the s: stale label Oct 25, 2019
@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Oct 25, 2019

Wow, a month has gone by already? It's still on my todo list to review...

@mlpack-bot mlpack-bot bot removed the s: stale label Oct 25, 2019
@zoq

This comment has been minimized.

Copy link
Member Author

zoq commented Oct 25, 2019

No worries.

Copy link
Member

rcurtin left a comment

Sorry it took me so long to get to this. It looks great to me (and the paper seems like a nicer idea than the one that switches from Adam to SGD after some number of iterations). Only a couple of comments about documentation and other little stuff and I think it's ready. 👍

* `AdaBound()`
* `AdaBound(`_`stepSize, batchSize`_`)`
* `AdaBound(`_`stepSize, batchSize, beta1, beta2, eps, maxIterations, tolerance, shuffle`_`)`
* `AdaBound(`_`stepSize, batchSize, beta1, beta2, eps, maxIterations, tolerance, shuffle, resetPolicy, exactObjective`_`)`

This comment has been minimized.

Copy link
@rcurtin

rcurtin Dec 11, 2019

Member

The parameters here don't seem to match the parameters in the table below.

doc/optimizers.md Outdated Show resolved Hide resolved
* `AdaBound()`
* `AdaBound(`_`stepSize, batchSize`_`)`
* `AdaBound(`_`stepSize, batchSize, beta1, beta2, eps, maxIterations, tolerance, shuffle`_`)`
* `AdaBound(`_`stepSize, batchSize, beta1, beta2, eps, maxIterations, tolerance, shuffle, resetPolicy, exactObjective`_`)`

This comment has been minimized.

Copy link
@rcurtin

rcurtin Dec 11, 2019

Member

Same comment here about the parameters matching the table below.

doc/optimizers.md Outdated Show resolved Hide resolved
include/ensmallen_bits/ada_bound/ada_bound.hpp Outdated Show resolved Hide resolved
doc/optimizers.md Show resolved Hide resolved
include/ensmallen_bits/ada_bound/ada_bound.hpp Outdated Show resolved Hide resolved
epsilon(epsilon),
beta1(beta1),
beta2(beta2),
iteration(0)

This comment has been minimized.

Copy link
@rcurtin

rcurtin Dec 11, 2019

Member

I think these should be doubly indented. 👍

zoq and others added 5 commits Dec 12, 2019
Co-Authored-By: Ryan Curtin <ryan@ratml.org>
Co-Authored-By: Ryan Curtin <ryan@ratml.org>
Co-Authored-By: Ryan Curtin <ryan@ratml.org>
Co-Authored-By: Ryan Curtin <ryan@ratml.org>
Copy link
Member

rcurtin left a comment

Just a couple more comments; I think it is basically ready. 👍

HISTORY.md Show resolved Hide resolved
doc/optimizers.md Show resolved Hide resolved
parent.iteration));

// Element wise maximum of past and present squared gradients.
vImproved = arma::max(vImproved, v.diag());

This comment has been minimized.

Copy link
@rcurtin

rcurtin Dec 13, 2019

Member

I think everything is correct except this, so if you want to revert 91b7653 or point out what I missed, then I think this is ready for merge. 👍

@zoq zoq force-pushed the zoq:AdaBound branch from 91b7653 to 681cef8 Dec 13, 2019
zoq added 3 commits Dec 13, 2019
@zoq

This comment has been minimized.

Copy link
Member Author

zoq commented Dec 13, 2019

Thanks for the review, I've merged the latest changes into the PR and updated HISTORY.md.

@coatless

This comment has been minimized.

Copy link
Contributor

coatless commented Dec 13, 2019

@rcurtin are you planning on rolling another release after this goes in? (And the k-step ahead / 1 step behind PR?)

@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Dec 14, 2019

@coatless yeah, I was. I was hoping to review the k-step PR, and if that looks like it will be a quick merge (it probably will, just guessing) then I'll wait until then to release ensmallen 2.11.0. I guess you are probably trying to decide whether to package 2.10.5? :)

Copy link
Member

rcurtin left a comment

Looks good to me. I found one small typo though for the definition of AMSBound. :) I seriously doubt that the tests will fail once that's made though---so I can make that change during merge and test at that time, or you can do it now, up to you. 👍

Co-Authored-By: Ryan Curtin <ryan@ratml.org>
@coatless

This comment has been minimized.

Copy link
Contributor

coatless commented Dec 14, 2019

I guess you are probably trying to decide whether to package 2.10.5? :)

@rcurtin that am I .

@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Dec 14, 2019

@coatless I think we can aim for a release in roughly a week. Hope that helps the decision. :)

@mlpack-bot
mlpack-bot bot approved these changes Dec 15, 2019
Copy link

mlpack-bot bot left a comment

Second approval provided automatically after 24 hours. 👍

@rcurtin rcurtin merged commit 9efcda9 into mlpack:master Dec 24, 2019
3 checks passed
3 checks passed
Static Code Analysis Checks Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
mlpack master build test Build finished.
Details
@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Dec 24, 2019

Awesome, excited to have this new optimizer available. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.