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

Deduplicate solver regularization, logging, and local rates and decays #2518

Merged
merged 3 commits into from
May 27, 2015

Conversation

shelhamer
Copy link
Member

This simplifies the solver code by de-duplicating shared logic.

I plan to merge this shortly to make way for an updated #1977.

@jeffdonahue
Copy link
Contributor

I just looked over this -- looks great! This refactoring was much needed. Thanks @cypof and @shelhamer. Only issue I see is I'm not sure about the verb in MakeUpdate -- I think DoUpdate or PerformUpdate would be more suggestive of what it does. Personally when I first saw the name MakeUpdate in the PR text, I guessed its purpose was that of ComputeUpdateValue.

@jeffdonahue
Copy link
Contributor

Actually, I guess it is replacing ComputeUpdateValue, but also doing net_->Update() at the end. Would it not be better to keep calling it ComputeUpdateValue and just keep net_->Update at the end of the body of Step?

@cypof
Copy link
Member

cypof commented May 27, 2015

No the net_->Update() needs to be in, so that it is not executed by solvers that are not of type SGDSolver. This way, only the root solver in a parallel setup will apply the update. That was actually the race I introduced when I split the big PR into small ones. Another name could be ApplyGradients?

@jeffdonahue
Copy link
Contributor

Okay, I see -- thanks for the explanation @cypof. I like Apply but also like Update; how about ApplyUpdate?

Designate `Solver::ApplyUpdate()` as the core method to compute
and apply parameter updates given the current state of the Net.

Make `Solver::ComputeUpdateValue()` a subordinate call overloaded by the
`SGDSolver`s to take care of optimization algorithm details.
@shelhamer
Copy link
Member Author

Thanks for the comments @cypof and @jeffdonahue. I went with ApplyUpdate. Please merge if this looks right to you, Jeff.

@shelhamer
Copy link
Member Author

p.s. ignore the travis push check -- the travis Pr check is the one to heed. The push check was triggered by my accidental push to BVLC/caffe and then my deleting the branch made it fail.

@jeffdonahue
Copy link
Contributor

Cool, LGTM

jeffdonahue added a commit that referenced this pull request May 27, 2015
Deduplicate solver regularization, logging, and local rates and decays
@jeffdonahue jeffdonahue merged commit b12c171 into BVLC:master May 27, 2015
@shelhamer shelhamer deleted the dedup_solvers branch May 27, 2015 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants