-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
"weights" added to solver parameters, "snapshot_prefix" field default initialization #6123
Conversation
Thank you! I am not shure it's a good idea to keep all changes related to #6110 in this one PR because 1) they are absolutely independent and 2) I'm not shure that I'll have possibility to implement all them at once. So shep-by-step approach looks better. |
I've squashed commits i this PR. I've found that python support of specific "weights" property is quite useless until python interface to solver will be improved in total. So this PR looks to be completed and ready for review. But some tests failed with messages like these: It seems not to be connected to my changes but rather to som network or configuration problems of build server. I have no idea what to do with it, even I have no possibility to retry them. Can you suggest? |
Along the way I've added implementation of improvement No.2 from issue #6110. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution, @IlyaOvodov! I left one comment in the code - let's discuss that and merge this soon.
src/caffe/proto/caffe.proto
Outdated
// If command line --weights parameter if specified, it has higher priority | ||
// and owerwrites this one. | ||
// If --snapshot command line parameter is specified, this one is ignored. | ||
optional string weights = 42; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better (or at least more Caffe-style) to do this as a repeated
field? I don't mind having an option to pass comma-separated values, but being able to use separate entries for several models will help avoid chaos (especially when using long absolute paths).
Loading weights is moved from caffe.exe to solver class, so new "weights" solver parameter is used not only from command line but when caffe is used as library (including python) corrected formatting fixed line length more formatting corrected
…points to a directory. See issue BVLC#6110 proposed improvement No.2
@Noiredd , I agree with your idea and decided that to make appropriate changes is better than to discuss it. |
Looks good. Thank you for the improvement, @IlyaOvodov! |
"weights" added to solver parameters, "snapshot_prefix" field default initialization
There are these 2 reasons:
|
"weights" added to solver parameters, "snapshot_prefix" field default initialization
This pull request implements parts 1&2 of feature request #6110.
"weights" parameter is added to SolverParameters class. It has quite the same meaning as "--weights" command line parameter of caffe executable and is used if "--weights" command line parameter is not specified. It makes it possible to specify initial weights in a solver.prototxt file instead of command line and make it unnecessary to change command line or command file.
It is aimed to make it possible to localize all training parameters in a solver.prototxt file. See #6110 for more detailed motivation.
If a "--snapshot" command line parameter is specified, new "weights" parameter in a solver.prototxt file is ignored. So there is no need to modify this file to continue previously interrupted training process from a snapshot.
EDIT by @Noiredd: second important contribution of this PR is a default initialization of the "solver_prefix" parameter. Previously Caffe would fail on parsing the solver if a snapshot was requested but no prefix given.