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

Add control of Reset Policy to Adam optimizers #60

Merged
merged 6 commits into from Dec 30, 2018

Conversation

@dtimson
Copy link
Contributor

commented Dec 3, 2018

Seems to be needed to do "single-epoch loop" training (see mlpack issue #1455) without resetting each time.

[Duplicate of a PR in mlpack.]

@rcurtin

rcurtin approved these changes Dec 3, 2018

Copy link
Member

left a comment

Thanks for the contribution! 👍

This looks good to me. During merge I'd like to add resetPolicy to the list of constructor arguments and update docs/optimizers.md accordingly.

Would you like me to add your name to the list of contributors?

@zoq

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

It would be a good idea to add the resetPolicy option to the rest of the SGD based optimizers as well e.g. RMSProp. @dtimson do you like to take a look into this?

@dtimson

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

  • Adding resetPolicy to the ctor args is OK, but it's only useful is we make another change. Currently, calling SGD::optimize() for the first time, when ResetPolicy==false, leads to an index out of bounds exception in the uninitialized UpdatePolicy. I've looked at a few options, and I'd like to suggest keeping a boolean 'isInitialized' flag in SGD. I could put in a PR to show you what I mean, if you like.

  • Yes, I'd be happy for you to add my name to the list of contributors. Thank you.

  • I agree that making all the SGD-based optimizers consistent by adding resetPolicy is a good idea. I've counted 6 such optimizers in the code: ada_delta, ada_grad, adam, rmsprop, smorms3, wn_grad. I'm happy to make the changes. For the documentation, I guess there's 11 entries to change (with the adam variants).

@rcurtin

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

You're right, we need to make sure updatePolicy gets initialized at least once. So an isInitialized flag seems like the best solution to me.

I'll add your name during merge---thanks again! Once this is merged I'll release ensmallen 1.12.0 with these changes.

Also, for mlpack first-time contributors we always offer to put some mlpack stickers in the mail that you could put on your laptop. But we don't have any ensmallen stickers... but if you want some mlpack stickers anyway I am happy to mail them to you; just send an email to ryan@ratml.org with your mailing address and I will get them sent out. :)

@rcurtin

This comment has been minimized.

Copy link
Member

commented Dec 26, 2018

@dtimson: just wanted to check in, were you still planning to handle all the SGD variants here? If not I can do it, just let me know.

@dtimson

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2018

My apologies - I had got the impression you wanted to handle that yourself during merge. I'm happy to do it, leave it with me. To clarify, I will now: add ResetPolicy() to the optimizers; add resetPolicy to ctor args of the optimizers; add isInitialized flag to SGD; update optimzers.md doc file.

@zoq

This comment has been minimized.

Copy link
Member

commented Dec 27, 2018

@dtimson Sounds like a good plan to me, looking forward to merge everything.

@rcurtin

This comment has been minimized.

Copy link
Member

commented Dec 27, 2018

Ah, oops, sorry for the miscommunication! If you can handle it it would be great. Looking forward to the patch. 👍

@dtimson

This comment has been minimized.

Copy link
Contributor Author

commented Dec 30, 2018

OK, I think the latest push has all the changes needed.

@@ -71,7 +71,8 @@ class AdaDelta
const double epsilon = 1e-6,
const size_t maxIterations = 100000,
const double tolerance = 1e-5,
const bool shuffle = true);
const bool shuffle = true,
const bool resetPolicy = true);

This comment has been minimized.

Copy link
@zoq

zoq Dec 30, 2018

Member

Can you add a description for the resetPolicy? That applies to the other optimizers as well.

This comment has been minimized.

Copy link
@dtimson

dtimson Dec 30, 2018

Author Contributor

Thanks. All done now.

@@ -144,6 +145,11 @@ class SnapshotSGDR
return optimizer.DecayPolicy().Snapshots();
}

//! Get whether or not to accumulate the snapshots.
bool Accumulate() const { return accumulate; }

This comment has been minimized.

Copy link
@zoq

zoq Dec 30, 2018

Member

Nice catch 👍

@rcurtin
Copy link
Member

left a comment

Looks good to me if you can handle the minor addition Marcus mentioned. Thanks for taking the time to do this! 👍

@zoq: I noticed that the SWATS optimizer has no documentation in optimizers.md. I'll open a PR momentarily to add that.

Once this (and that) are merged, I'll release ensmallen 1.12.0. 👍

@zoq zoq merged commit 7275baf into mlpack:master Dec 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zoq

This comment has been minimized.

Copy link
Member

commented Dec 30, 2018

Thanks again for the contribution 👍

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.