Conversation
|
@philkr could you review this if you have a chance? |
src/caffe/proto/caffe.proto
Outdated
There was a problem hiding this comment.
Why not use momentum here, to be consistent with other solvers?
|
@PatWie thanks for the solver! All SGD solvers need gradient checks. See for instance the AdaGrad tests https://github.com/BVLC/caffe/blob/master/src/caffe/test/test_gradient_based_solver.cpp#L431-L483 |
src/caffe/solver.cpp
Outdated
There was a problem hiding this comment.
Why divide by stepsize here?
There was a problem hiding this comment.
I think t is the epoch rather than the iteration by the definition of Caffe.
|
@shelhamer Ah. i didn't realized that there is already a unit-test. I will add one of course. |
src/caffe/solver.cpp
Outdated
There was a problem hiding this comment.
The three commands above can be written as a single caffe_cpu_axpby using beta1 instead of 0 and val_m_ instead of val_t_.
There was a problem hiding this comment.
Ah I see. Blas is completely new for me. I will change this tomorrow at all places in my code.
|
I think there's some confusion going on in the code due to the usage of the stepsize param. Which is made worse by using lr_policy "step" in the MNIST example. The alpha/stepsize from the paper should be set via the base_lr, and used with lr_policy: "fixed" as I don't see any recommendations for changing alpha during training. This way you can also get rid of gamma and power in the prototxt (the latter wasn't being used anyway). The stepsize param should only be used together with lr_policy "step", and if we already set the alpha via base_lr then it is not needed at all. t can just be iter_ +1, as it's afaik not needed to compute the effective stepsize. This also removes the need for a check if stepsize > 0 in the header. Moreover, it makes sense to change the MNIST example to use the recommended value from the paper for the base_lr (0.001) and explicitly set momentum and momentum2 to 0.9 and 0.999 respectively, rather than relying on the default values. |
|
I applied all changes in memory usage, solver_mnist proto,
|
src/caffe/solver.cpp
Outdated
There was a problem hiding this comment.
A reference to a shared pointer is never good idea, just copy the pointer.
|
The solver looks good to me now. I haven't tested it though. I do think that caffe_ipow should be it's own PR if we really want to add it. Currently it just bloats this PR request and doesn't add any benefit (see https://en.wikipedia.org/wiki/Amdahl%27s_law). |
src/caffe/solver.cpp
Outdated
There was a problem hiding this comment.
please fix indentation -- use 4 space indents when continuing from previous lines (https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Spaces_vs._Tabs)
|
@PatWie Thank you for this great PR! I just added some comment on the code. Please fix indent to 4 space indents when continuing from previous lines, and add more test cases. After that, squash into one commit and I can merge. |
6 participants
This commit implements the Adam solver by Kingma et. al for CPU and
GPU. All solver parameters are defined in the caffe.proto. This also
adds an example for the MNIST dataset.
see issue #2827
Before merging, please review the code. I will add changes to this branch (and rebase) if there should be something to change.