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

add clarabel #61

Merged
merged 9 commits into from
Oct 21, 2023
Merged

add clarabel #61

merged 9 commits into from
Oct 21, 2023

Conversation

sbarratt
Copy link
Collaborator

@sbarratt sbarratt commented Aug 30, 2023

this passes the tests. i had to lower the atol in the exp_cone test a bit; might be worth looking into.

@PTNobel
Copy link
Member

PTNobel commented Aug 30, 2023

Sorry lower atol as in make it smaller? So we're requiring a more accurate answer?

@sbarratt
Copy link
Collaborator Author

sbarratt commented Aug 30, 2023 via email

@sbarratt
Copy link
Collaborator Author

I can't figure it out. did some basic checking, but Clarabel gets a better solution than SCS on both the initial solve and also the perturbed solve, so Im king of scratching my head. @bstellato

@bungun
Copy link

bungun commented Aug 31, 2023 via email

@bstellato
Copy link
Member

Could it be an issue of the linear system solved to take the derivative?

Clarabel returns a higher accuracy solution with smaller complementary slackness residuals. It may cause the linear system to be badly scaled and lsqr to return less accurate derivatives. Just a guess though...

@sbarratt
Copy link
Collaborator Author

sbarratt commented Sep 4, 2023 via email

@bstellato
Copy link
Member

There were two issues with Clarabel

  • It was not using the settings passed
  • Its equilibration was making it way less precise

It now works! @phschiele @akshayka @sbarratt what do you think?

@sbarratt
Copy link
Collaborator Author

sbarratt commented Oct 20, 2023 via email

Copy link
Collaborator

@phschiele phschiele left a comment

Choose a reason for hiding this comment

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

@sbarratt @bstellato Thanks for this PR! I think it is really important to make sure that the main cvxpy extensions support Clarabel to foster adoption in the community.

I suggest to remove two small debug statements introduced during development.

Could it make sense to disable equilibrate by default if it was causing such trouble in the tests?

tests/test_scs.py Outdated Show resolved Hide resolved
tests/test_scs.py Outdated Show resolved Hide resolved
@bstellato
Copy link
Member

Thanks @phschiele. I have removed the print comments (sorry I did not see them before) and reverted the equilibration settings to its default ones. It now works as any other solver, I would not disable equilibration by default. It should be good to go now.

@phschiele
Copy link
Collaborator

@bstellato Thanks! Let's wait for @akshayka 's review. I'm updating the build in #63, so we can go ahead an cut a release once this is merged.

@phschiele phschiele merged commit eaac7bc into master Oct 21, 2023
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.

6 participants