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

Mosek uses imposes linear or rotated quadratic constraint for small-size LMI constraints. #22238

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hongkai-dai
Copy link
Contributor

@hongkai-dai hongkai-dai commented Nov 25, 2024

For LMI on a 1x1 matrix, we impose linear inequality constraint. For LMI on a 2x2 matrix, we impose a rotated quadratic constraint (rotated Lorentz cone constraint)


This change is Reviewable

@hongkai-dai hongkai-dai added the release notes: feature This pull request contains a new feature label Nov 25, 2024
Copy link
Contributor Author

@hongkai-dai hongkai-dai left a comment

Choose a reason for hiding this comment

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

+@AlexandreAmice for feature review please, thanks!

Reviewable status: LGTM missing from assignee AlexandreAmice, needs platform reviewer assigned, needs at least two assigned reviewers

Copy link
Contributor

@AlexandreAmice AlexandreAmice left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee AlexandreAmice, needs platform reviewer assigned, needs at least two assigned reviewers


solvers/mosek_solver_internal.h line 651 at r1 (raw file):

        const double tmp = dual_sol(1);
        dual_sol(1) = dual_sol(2) / 2;
        dual_sol(2) = tmp;

I find this inconsistent reshuffling a bit confusing. I would introduce the mapping

[y0, y1] -> [y0, y2, y1*sqrt(2)]
[y1, y2]

this being in mosek's rotated lorentz cone is the condition

2*y0*y2 >=  2 * y1**2 

which is the determinant condition and this is just a reshuffling of the standard PSD matrix vectorization.

Then denote the dual solution of Mosek's rotated second order cone as

[z0, z2, z1]

which makes it clear that under the inverse mapping, to preserve the inner product we must have

[z0, z2, z1] -> [z0, z1/sqrt(2)]
                       [z1/sqrt(2), z2]

This makes everything symmetric and much more similar to the normal case.

Suggestion:

        // We impose the LMI constraint as a rotated Quadratic cone constraint
        // in Mosek. For a constraint that
        // [y0 y1]
        // [y1 y2]
        // is in psd, this is equivalent to
        // [y0/2, y2, y1] in Mosek's rotated quadratic cone.
        // If we denote the dual solution of the Mosek's rotated quadratic cone
        // as [z0, z1, z2], then by the duality theory, that the complementary
        // slackness (namely the inner product between the primal and the dual)
        // should be the same for both the PSD cone and the rotated quadratic
        // cone, hence the dual solution for the PSD constraint is
        // [z0/2 z2/2]
        // [z2/2   z1]
        // We store the lower triangular part of the dual PSD matrix as the dual
        // solution, namely [z0/2, z2/2, z1]
        dual_sol(0) *= 0.5;
        const double tmp = dual_sol(1);
        dual_sol(1) = dual_sol(2) / 2;
        dual_sol(2) = tmp;  

solvers/mosek_solver_internal.cc line 969 at r1 (raw file):

      A.setFromTriplets(A_triplets.begin(), A_triplets.end());
      MSKdomaintypee domain_type =
          matrix_rows == 1 ? MSK_DOMAIN_RPLUS : MSK_DOMAIN_SVEC_PSD_CONE;

It might be nice to hard-code the 1x1 case similar to the 2x2 case. It would help the reader separate the three cases more easily.

Code quote:

      MSKdomaintypee domain_type =
          matrix_rows == 1 ? MSK_DOMAIN_RPLUS : MSK_DOMAIN_SVEC_PSD_CONE;

solvers/mosek_solver_internal.cc line 981 at r1 (raw file):

      // [y1, y2]
      // We impose it as a rotated Lorentz cone constraint, namely
      // [y0/2, y2, y1] is in Mosek's rotated Quadratic cone.

see discussion on the dual solution.

Code quote:

      // For PSD constraint on a 2x2 matrix
      // [y0, y1] = F0 + F1*x0 + ... + Fk*xk
      // [y1, y2]
      // We impose it as a rotated Lorentz cone constraint, namely
      // [y0/2, y2, y1] is in Mosek's rotated Quadratic cone.

Copy link
Contributor Author

@hongkai-dai hongkai-dai left a comment

Choose a reason for hiding this comment

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

Reviewable status: 3 unresolved discussions, LGTM missing from assignee AlexandreAmice, needs platform reviewer assigned, needs at least two assigned reviewers


solvers/mosek_solver_internal.h line 651 at r1 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

I find this inconsistent reshuffling a bit confusing. I would introduce the mapping

[y0, y1] -> [y0, y2, y1*sqrt(2)]
[y1, y2]

this being in mosek's rotated lorentz cone is the condition

2*y0*y2 >=  2 * y1**2 

which is the determinant condition and this is just a reshuffling of the standard PSD matrix vectorization.

Then denote the dual solution of Mosek's rotated second order cone as

[z0, z2, z1]

which makes it clear that under the inverse mapping, to preserve the inner product we must have

[z0, z2, z1] -> [z0, z1/sqrt(2)]
                       [z1/sqrt(2), z2]

This makes everything symmetric and much more similar to the normal case.

I agree scaling y2 by sqrt(2) is aesthetically better (it is symmetric).

On the other hand, all the rotated Lorentz cone constraint in MosekSolver is parsed by scaling y0 with a factor of 0.5. To stay consistent with all the other code in side this same file, I think we should do scaling of 0.5 on y0 here as well.

We could have a separate PR to change the scaling for all rotated Lorentz cone constraint.


solvers/mosek_solver_internal.cc line 981 at r1 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

see discussion on the dual solution.

Done. Ditto

Copy link
Contributor

@AlexandreAmice AlexandreAmice left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers


solvers/mosek_solver_internal.h line 651 at r1 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

I agree scaling y2 by sqrt(2) is aesthetically better (it is symmetric).

On the other hand, all the rotated Lorentz cone constraint in MosekSolver is parsed by scaling y0 with a factor of 0.5. To stay consistent with all the other code in side this same file, I think we should do scaling of 0.5 on y0 here as well.

We could have a separate PR to change the scaling for all rotated Lorentz cone constraint.

Very well. I would still adjust the labeling in the comment so that it appears as [z0 z2 z1]

…ize LMI constraints.

For LMI on a 1x1 matrix, we impose linear inequality constraint.
For LMI on a 2x2 matrix, we impose a rotated quadratic constraint (rotated Lorentz cone constraint)
Copy link
Contributor Author

@hongkai-dai hongkai-dai left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers


solvers/mosek_solver_internal.h line 651 at r1 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

Very well. I would still adjust the labeling in the comment so that it appears as [z0 z2 z1]

Done.


solvers/mosek_solver_internal.cc line 969 at r1 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

It might be nice to hard-code the 1x1 case similar to the 2x2 case. It would help the reader separate the three cases more easily.

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: feature This pull request contains a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants