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

Solver Refactor: Separate files and Change Solver's Type to String #3116

Merged
merged 4 commits into from
Oct 17, 2015

Conversation

ronghanghu
Copy link
Member

Resolve #2890.

Split solver.cpp into each solver per file, allow self-contained solver development, and provide abstraction for adaptive gradient solvers.

Caffe has individual cpp files for each layer, and in #1694, layer types were changed to strings to allow self-contained layer development. Similar work should be done for solvers, especially given that right now all solver implementations are in solver.cpp and their tests in test_gradient_based_solver.cpp, both 1200+ lines.

Part 1: Solver Refactor

  • Split solver implementation into each file per solver, and add solver factory
  • Change solver type to strings, add layer-style solver factor and solver registry mechanism
  • Implement automatic upgrade for existing solver files
  • Update examples and docs

Note: The old enum field solver_type is changed to the new string field type in SolverParameter. Automatic upgrade provided.

Part 2: Solver Abstraction (in another PR)

  • Extract common parts in adaptive gradient-based solvers and provide abstract adaptive gradient solver class
  • Rewrite solver tests. Split and provide abstraction for solver tests to remove duplicated code.

@ronghanghu
Copy link
Member Author

To provide correct level of solver abstractions, I hope the hierarchy would be

/Solver (abstract class)
/Solver/BaseSGDSolver (abstract class)
/Solver/BaseSGDSolver/SGDSolver
/Solver/BaseSGDSolver/NesterovSolver
/Solver/BaseSGDSolver/AdaptiveSGDSolver (abstract class)
/Solver/BaseSGDSolver/AdaptiveSGDSolver/AdamSolver
/Solver/BaseSGDSolver/AdaptiveSGDSolver/RMSPropSolver
/Solver/BaseSGDSolver/AdaptiveSGDSolver/AdaGradSolver
/Solver/BaseSGDSolver/AdaptiveSGDSolver/AdaDeltaSolver
/Solver/HessianFreeSolver (abstract class)
/Solver/HessianFreeSolver/SomeToBeImplementedHessianFreeSolver

Any suggestions are welcome!

@ronghanghu
Copy link
Member Author

@shelhamer Do you prefer that I address all the above tasks in one PR, or should I split them into separate PRs (I am in favor of the latter)?

@shelhamer
Copy link
Member

@ronghanghu you are welcome to do it as a progression but certain tasks need to be done together such as

  • Change solver type to strings, add layer-style solver factor and solver registry mechanism
  • Implement automatic upgrade for existing solver files

so that each PR yields a correct master. Thank you for bringing order to the solver code!

@ronghanghu ronghanghu force-pushed the solver-refactor branch 2 times, most recently from d1e18af to 798ce46 Compare September 25, 2015 05:11
@ronghanghu
Copy link
Member Author

I feel it is better to split this PR into two separate PRs, one for splitting files and one for solver abstraction. Part 1 should be (almost) completed.

@ronghanghu ronghanghu changed the title Solver Refactor: Separate files, Self-contained Development and Enhance Abstraction Solver Refactor (Part 1): Separate files and Change Solver's Type to String Sep 25, 2015
@ronghanghu ronghanghu force-pushed the solver-refactor branch 8 times, most recently from 88f0830 to 3334fdb Compare September 25, 2015 07:30
@ronghanghu ronghanghu changed the title Solver Refactor (Part 1): Separate files and Change Solver's Type to String Solver Refactor: Separate files and Change Solver's Type to String Sep 25, 2015
@ronghanghu ronghanghu force-pushed the solver-refactor branch 2 times, most recently from 5244a8d to 37755b4 Compare September 25, 2015 16:56
@ronghanghu ronghanghu force-pushed the solver-refactor branch 4 times, most recently from 9692f71 to ec972e8 Compare September 26, 2015 18:47
@ronghanghu
Copy link
Member Author

A rebase is needed here after #3204.

@shelhamer
Copy link
Member

@ronghanghu sorry about that! I'm happy to do a last review and merge after the rebase so we can have orderly solver code.

@ronghanghu
Copy link
Member Author

Rebased to the latest master

shelhamer added a commit that referenced this pull request Oct 17, 2015
Solver Refactor: Separate files and Change Solver's Type to String
@shelhamer shelhamer merged commit 16de340 into BVLC:master Oct 17, 2015
@shelhamer
Copy link
Member

Thanks for splitting up the solvers and decoupling development Ronghang!

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.

2 participants