Skip to content

Comments

Enables the alternate solvers to be accessed by the python interface.#3024

Merged
shelhamer merged 1 commit intoBVLC:masterfrom
danielgordon10:python-solver-fix
Sep 4, 2015
Merged

Enables the alternate solvers to be accessed by the python interface.#3024
shelhamer merged 1 commit intoBVLC:masterfrom
danielgordon10:python-solver-fix

Conversation

@danielgordon10
Copy link

No description provided.

@shelhamer
Copy link
Member

Thanks for including more solvers @danielgordon10, but please revise to add AdaDeltaSolver too.

@jeffdonahue
Copy link
Contributor

I'll note that the other solvers can already be accessed by get_solver. But given that we already directly expose SGDSolver, it would be more consistent to expose the others as well. (An alternate route to consistency would be to remove SGDSolver and always use get_solver; I don't have a strong opinion either way.)

@shelhamer
Copy link
Member

@jeffdonahue right -- I'm fine with either exposing them all like this or documenting the use of get_solver() even though SGDSolver is a special case by its popularity.

@danielgordon10
Copy link
Author

@shelhamer Whoops, totally missed that one.
Yeah, I saw get_solver at the tail end of my debugging, after I had written the other exposers. I appreciate the explicitness of Solver though, especially because SGDSolver exists. I think if you want to encourage get_solver, you shouldn't even have SGDSolver as its own method.

@danielgordon10
Copy link
Author

Either way, I'm pretty sure the NesterovSolver and AdaGradSolver lines in _caffe.cpp were uncallable without a change. So something needs to be done.

@danielgordon10
Copy link
Author

Anyone know what the build fail is about?

@shelhamer
Copy link
Member

From the travis log:

[  FAILED  ] 1 test, listed below:
[  FAILED  ] AccuracyLayerTest/1.TestForwardCPUPerClass, where TypeParam = double

which blame points to 374fb8c in #2935. @ronghanghu could you check what this is about?

@danielgordon10 the test failure has nothing to do with your PR, so don't worry about that. However do squash your fixup commit for AdaDelta for a clear history. Thanks.

@ronghanghu
Copy link
Member

@shelhamer I'll checkout shortly.

@ronghanghu
Copy link
Member

@shelhamer There is a bug with AccuracyLayer test in #2935 which I didn't observe. It was my fault. I made a fix PR in #3027.

@danielgordon10
Copy link
Author

@shelhamer Is there an easy way to squash the commits into an already pushed branch? I'm having issues pushing the squashed version saying I'm behind the pushed one. And if I pull first, then I have a bunch of commits, which is exactly what squash is trying to avoid.

@shelhamer
Copy link
Member

@danielgordon10 once you squash, force push by git push -f to your PR branch to update the PR.

@danielgordon10
Copy link
Author

@shelhamer naughty naughty, using -f. I'll do it, but I won't enjoy it.

@shelhamer
Copy link
Member

Not at all -- "published history" in the sense of the project is the canonical BVLC/caffe master branch. Force push is not only fine but right to wrap up PRs.

shelhamer added a commit that referenced this pull request Sep 4, 2015
[pycaffe] expose all solvers for direct instantiation (although note get_solver)
@shelhamer shelhamer merged commit 5367a1a into BVLC:master Sep 4, 2015
ctrevino added a commit to Robotertechnik/caffe that referenced this pull request Sep 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants