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

"Nadam" and "AMSGrad" are implemented. #450

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

askrix
Copy link

@askrix askrix commented Oct 12, 2018

"Nadam" and "AMSGrad" are implemented. ;-)

@codecov
Copy link

codecov bot commented Oct 12, 2018

Codecov Report

Merging #450 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #450   +/-   ##
=======================================
  Coverage   92.78%   92.78%           
=======================================
  Files          40       40           
  Lines        1372     1372           
=======================================
  Hits         1273     1273           
  Misses         99       99
Impacted Files Coverage Δ
src/train.jl 99.06% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ef1357...5e4493c. Read the comment docs.

@oxinabox oxinabox changed the title Update train.jl "Nadam" and "AMSGrad" are implemented. Oct 14, 2018
@oxinabox
Copy link
Collaborator

This looks promising, thanks. Nice work diving into a fairly deep part of the system.
Can you add some tests?

@malmaud should probably do the code review, I'm not very familiar with this part of the code myself.

@askrix
Copy link
Author

askrix commented Oct 14, 2018

Mr. White,

many thanks for your feedback. I tried my best to get the implementation done. As I wrote you about it in issue #447 , I tested the implementation on my computer using Himmelsbau's function and compared the number of iterations each algorithm needed to converge by the given precision and other parameters.

How should look a test for the algorithms in "TensorFlow.jl"? Should I write for example a linear regression task, where different optimization algorithms are benchmarked against each other?

In connection with my pool request I'd like to ask if it will be possible to reorganize the source code and to export all of the optimization algorithms from "train.jl" in the future. For example by creating an extra sub-folder so that each optimizer will have an own file. (E.g. "Nadam.jl"). I'm asking for it because I'm planning to add doc""" some explanations and notes """ to reference to the publications on which my implementations are based.

Best regards,
Askrix

@askrix
Copy link
Author

askrix commented Nov 12, 2018

@malmaud Mr. Malmaud,

so far I still didn't get any further feedback regarding my pull-request. I guess my implementation is either not sufficient good or/and something is missing. Could you tell me what I should correct or/and add to the code?

In the meanwhile I prepared the following optimizers: AdaGrad, Adadelta, RMSProp and AdaMax. Should I open a new pull request?

Best regards,
Askrix

@malmaud
Copy link
Owner

malmaud commented May 17, 2019

Sorry for the delay, @askrix. These actually look really good! We need some kind of test, even if it's informal, to make sure these work.

I propose adding tests that run the 'logistic.jl' example, but with your new optimizers instead of the Adam optimizer used now. Then we can at least check that the error goes to near zero. You could just modify logistic.jl to be a function that takes an optimizer constructor as an argument and then modify runtests.jl to pass in all the new optimizers to the logistic example. I'm open to other testing ideas as well.

How does that sound?

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.

3 participants