-
Notifications
You must be signed in to change notification settings - Fork 55
Expire CVXOPT Dependency #588
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
base: develop
Are you sure you want to change the base?
Conversation
Switches to using Clarabel as the solver for diamond distance and adds a minimum version pin for cvxpy.
pygsti/tools/optools.py
Outdated
try: | ||
prob.solve(solver='CVXOPT') | ||
prob.solve(solver='Clarabel') | ||
except _cvxpy.error.SolverError as e: | ||
_warnings.warn("CVXPY failed: %s - diamonddist returning -2!" % str(e)) | ||
return (-2, _np.zeros((dim, dim))) if return_x else -2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do the following instead
objective_val = -2
varvals = [_np.zeros_like(J), None, None]
sdp_solvers = ['MOSEK', 'CLARABEL', 'CVXOPT']
for i, solver in enumerate(sdp_solvers):
try:
prob.solve(solver=solver)
objective_val = prob.value
varvals = [v.value for v in vars]
break
except (AssertionError, _sdps.cp.SolverError) as e:
msg = f"Received error {e} when trying to use solver={solver}."
if i + 1 == len(sdp_solvers):
failure_msg = "Out of solvers. Returning -2 for diamonddist."
else:
failure_msg = f"Trying {sdp_solvers[i+1]} next."
msg += f'\n{failure_msg}'
_warnings.warn(msg)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I left almost the same comment verbatim in some other GitHub interface. I'm not sure where that comment went.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the import for _sdps
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, whoops. It's on a branch where I made an sdptools.py file. All that matters is that _sdps.cp.SolverError
is cvxpy.SolverError
. So it'll suffice to import cvxpy as cp and then replace with cp.SolverError
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I made this change with a small modification to skip producing warnings is MOSEK isn't installed, since we can assume this is pretty much always going to be the case for most users. (Read: I don't have MOSEK and didn't want to get constant warnings about that...)
While going through I also noticed that there was a flag disabling the diamond distance unit test on windows that was added about 3 years ago. The commit message on that indicated it was due to random CVXOPT failures on the runners. I'm going to blindly assume that the changes we're making plus the intervening three years probably fixed things on their own so reenabled those.
Add in multiple solver options that are sequentially tried before fully failing. Also turn back on diamond distance tests on windows which were previously being skipped.
This finishes the expiration of the dependency on CVXOPT that we announced in the release of 0.9.13. The cvxpy dependency has had an associated minimum version requirement added.
See #437 for more.