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

Adaptive Solvers: AdaDelta, RMSprop, and ADAM #2860

Closed
3 tasks done
shelhamer opened this issue Aug 4, 2015 · 8 comments
Closed
3 tasks done

Adaptive Solvers: AdaDelta, RMSprop, and ADAM #2860

shelhamer opened this issue Aug 4, 2015 · 8 comments

Comments

@shelhamer
Copy link
Member

There are open PRs for adaptive solvers that need review, revision, and merge:

As a last step AdaGrad, AdaDelta, and RMSprop could likely be improved by refactoring out shared code into an AdaptiveSGDSolver

@ronghanghu these could be good warm-up projects for development.

@longjon @jeffdonahue thoughts on posting tracking issues of this kind? Does this help navigate or is it noise?

@shelhamer shelhamer added the focus label Aug 4, 2015
@ronghanghu
Copy link
Member

OK, I'll go through these PRs. Also, I agree that solver_type should be string instead of enum.

@matthiasplappert
Copy link
Contributor

AdaDelta should already conform to the new solver interface. Unless I missed something, I ported it over in #2782 when merging in master a couple of weeks ago.

@ronghanghu
Copy link
Member

Since right now these 3 PRs are blocking each other by enum value in SolverType and SolverParameter next available ID, I expect to merge Adam #2856 or AdaDelta #2782 first, and RMSprop afterwards. Merged #2867 first. If necessary, I can take over and clean up #2856 and #2782.

After that, a solver refactor PR can follow to address the solver_type and split solver.cpp into different files.

@ronghanghu ronghanghu added the RH label Aug 7, 2015
@ronghanghu
Copy link
Member

Note: #2836 and #2866 modifies solver as well as introduces new snapshot tests, so a further rebase is needed here. #2870 is also going to introduce new conflicts.

@bhack
Copy link
Contributor

bhack commented Aug 8, 2015

@ronghanghu The merging sequence It is becoming to be a "political" issue because there isn't a clear policy for reviews and merges. Fast track merges on younger PRs push adaptation work over the older one (apart of the quality, the in shape status, the historical rebasing effort a maintainer has done on this older PRs)

@ronghanghu
Copy link
Member

@bhack Indeed, there isn't a clear policy for PR reviewing process right now. Our merge sequence is mainly based on the status of PRs, that is, whether a PR is ready to merge or needs further modification. Since these 2 out of 3 here (Adam and AdaDelta) are still not ready, I expect to wait to merge them after #2870.

@bhack
Copy link
Contributor

bhack commented Aug 8, 2015

The problem is that there isn't almost a rough policy for the community. Readiness it is influenced also by reviews timing other that proposer availability. #2836 that brake this was a fast track review. There are many staging PR in the queue that need to support rebasing efforts of fast track Pr merges.

@ronghanghu
Copy link
Member

Now all of them done. Address solver refactor in #2890.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants