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

Migrate from cvxopt to cvxpy (+osqp / clarabel) #8

Merged
merged 6 commits into from
Mar 14, 2024
Merged

Conversation

phschiele
Copy link
Collaborator

@phschiele phschiele commented Mar 8, 2024

Checklist:

  • Add unit tests for current formulation
  • Migrate to CVXPY formulation

@fabian-sp fabian-sp added the under construction The PR is not yet finalized label Mar 8, 2024
@phschiele
Copy link
Collaborator Author

phschiele commented Mar 11, 2024

@fabian-sp Replaced cvxopt with cvxpy.
Right now, I basically skip initialization and update. Once we can verify correctness, I'll use cvxpy parameters to speed up repeated canonicalizations, which will again use the initialization/update/solve methods.

Equality constrained tests are still failing.

Update:
I think there might be a sign error in main?

@fabian-sp
Copy link
Owner

For the record: the -1 was indeed incorrect, should have been a +1.

@phschiele phschiele removed the under construction The PR is not yet finalized label Mar 12, 2024
@fabian-sp
Copy link
Owner

@phschiele Did you run scripts/time_rosenbrock.py to see how the solve time for the toy example changed?
I somehow can't run this anymore, getting the error

NameError: name 'problem' is not defined

@fabian-sp
Copy link
Owner

Comparing main to this branch on the rosenbrock timing, I get:

main: 163 ms ± 4.08 ms
cvxpy-subroblem: 850 ms ± 5.35 ms

So somehow there seems to be some overhead/slower solving time due to cvxpy.

Copy link
Owner

@fabian-sp fabian-sp left a comment

Choose a reason for hiding this comment

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

Left some questions/comments, but can be merged

@@ -26,7 +26,8 @@ classifiers = [
dependencies = [
"numpy",
"torch",
"cvxopt",
"cvxpy-base",
Copy link
Owner

Choose a reason for hiding this comment

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

Why not cvxpy?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CVXPY comes with multiple solvers by default. cvxpy-base is only the modeling language itself, and we only install one solver (clarabel) explicitly. This makes maintenance a lot easier.

H: np.ndarray,
rho: float,
D_f: np.ndarray,
D_gI: list[np.ndarray],
Copy link
Owner

Choose a reason for hiding this comment

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

My VS Code complains about the list, probably prefers the List form typing?

"""
The quadratic subrpoblem we solve in every iteration is of the form:
@property
def problem(self) -> cp.Problem:
Copy link
Owner

Choose a reason for hiding this comment

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

What is the ruleset on whether to define a property or to initialize in the __init__? To me this looks slightly overcomplicated...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did it this way to prevent users from having to check if the problem has been initialized yet every time, since in the __init__ you would set it to None, right?


from ncopt.sqpgs.main import SubproblemSQPGS


Copy link
Owner

Choose a reason for hiding this comment

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

Are the tested subproblems somehow special (that is, coming from a special case of the underlying problem)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They are the first iterates of the rosenbrock example, so not special.

@fabian-sp fabian-sp merged commit 5b20d40 into main Mar 14, 2024
2 checks passed
@fabian-sp fabian-sp deleted the cvxpy-subproblem branch March 14, 2024 10:43
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