Skip to content
This repository was archived by the owner on Jul 1, 2023. It is now read-only.

Add AdaMax optimizer #243

Closed
wants to merge 1 commit into from
Closed

Conversation

jon-tow
Copy link
Contributor

@jon-tow jon-tow commented Jun 14, 2019

Add implementation of AdaMax along with testing for correctness.

#127

Note: Build was broken with current macOS compiler (June 12, 2019 Snapshot) because of an unrelated call to move(along:) in the RiemannSGD optimizer. Tests pass when move is reverted to moved.

@rxwei rxwei requested review from rxwei and dan-zheng June 14, 2019 18:52

static var allTests = [
("testAdaGrad", testAdaGrad),
("testAdaMax", testAdaMax),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
("testAdaMax", testAdaMax),
("testAdaMax", testAdaMax)

Copy link
Contributor

@rxwei rxwei left a comment

Choose a reason for hiding this comment

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

Thanks @jon-tow for putting this together! We are in the process of simplifying optimizer definitions with aggregate vector operations and elementary functions (#218). Would you mind waiting until that work is finished first? That way, the definition of this optimizer can be greatly simplified.

@jon-tow
Copy link
Contributor Author

jon-tow commented Jun 14, 2019

@rxwei Sure thing! I'll put a hold on it until further notice.

@rxwei rxwei added the on hold label Jun 14, 2019
@jon-tow jon-tow closed this Jun 15, 2019
@jon-tow jon-tow deleted the optimizers/AdaMax branch June 15, 2019 07:12
@rxwei
Copy link
Contributor

rxwei commented Jun 15, 2019

You actually didn't need to close this PR. Our optimizer refactoring will be ready in a day or two :)

@jon-tow
Copy link
Contributor Author

jon-tow commented Jun 15, 2019

Unfortunately, this was the result of my inexperience with git reset hard on my local fork 😳. I’ll gladly submit another PR (as I’m unable to reopen this one) with updates based on the upcoming refactoring, if that’s the best option?

@rxwei
Copy link
Contributor

rxwei commented Jun 15, 2019

No worries at all! A new PR sounds good.

@Shashi456
Copy link
Contributor

@jon-tow You could put another PR with adamax now, #218 was merged.

@jon-tow jon-tow mentioned this pull request Jun 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants