Skip to content

Allow warm-start in diffcp#867

Merged
akshayka merged 4 commits into
cvxpy:masterfrom
bstellato:bs/diffcp_warm_start
Nov 13, 2019
Merged

Allow warm-start in diffcp#867
akshayka merged 4 commits into
cvxpy:masterfrom
bstellato:bs/diffcp_warm_start

Conversation

@bstellato
Copy link
Copy Markdown
Contributor

This PR allows to warm-start SCS in diffcp. Note that the warm_start option in diffcp now conflicts with the standard cvxpy warm_start option which is True/False.

Also, it would be good to check the type of the warm_start entry in diffcp to avoid errors:
https://github.com/cvxgrp/diffcp/blob/2e0d3f7337e19509bd312272ed59be1f93104ff2/diffcp/cone_program.py#L248-L251

@akshayka akshayka self-requested a review November 12, 2019 21:50
Comment thread cvxpy/reductions/solvers/conic_solvers/diffcp_conif.py Outdated
Copy link
Copy Markdown
Collaborator

@akshayka akshayka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! We should (only) support True/False warm-starting in CVXPY. See my comment.

@akshayka
Copy link
Copy Markdown
Collaborator

Looks great. One more thing (& probably the last thing) --- can you add a unit test that exercises the warm start code path? Something simple is fine, e.g., similar to https://sourcegraph.com/github.com/cvxgrp/cvxpy@af2f1e8eddb2997ac52d105f6e81d5d63582180d/-/blob/cvxpy/tests/test_scs.py#L186-196 (without the print statement).

The test could go in test_scs.py or test_derivative.py.

Also, flake8:
./cvxpy/reductions/solvers/conic_solvers/diffcp_conif.py:83:16: E111 indentation is not a multiple of four

Copy link
Copy Markdown
Collaborator

@akshayka akshayka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few small nits. I'll approve & merge once they're addressed. Thanks!

Comment thread cvxpy/tests/test_scs.py Outdated
result2 = prob.solve(solver=cp.DIFFCP, warm_start=True, eps=1e-4)
time2 = prob.solver_stats.solve_time
self.assertAlmostEqual(result2, result, places=2)
print(time > time2)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete the print statement, please.

cones = scs_conif.dims_to_solver_dict(data[ConicSolver.DIMS])
# Default to eps = 1e-4 instead of 1e-3.
solver_opts['eps'] = solver_opts.get('eps', 1e-4)
warm_start_dict = None
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warm_start_dict -> warm_start_tuple

Comment thread cvxpy/tests/test_scs.py Outdated
obj = cp.Minimize(cp.sum(cp.exp(x)))
prob = cp.Problem(obj, [cp.sum(x) == 1])
result = prob.solve(solver=cp.DIFFCP, eps=1e-4)
time = prob.solver_stats.solve_time
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete this line

Comment thread cvxpy/tests/test_scs.py Outdated
result = prob.solve(solver=cp.DIFFCP, eps=1e-4)
time = prob.solver_stats.solve_time
result2 = prob.solve(solver=cp.DIFFCP, warm_start=True, eps=1e-4)
time2 = prob.solver_stats.solve_time
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete this line

@bstellato
Copy link
Copy Markdown
Contributor Author

Should be done now. Let me know if there is anything else to change. Thanks!

Copy link
Copy Markdown
Collaborator

@akshayka akshayka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!+

@akshayka akshayka merged commit 18ddad5 into cvxpy:master Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants