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 parse PSD constraint on 2x2 matrices as RotatedLorentzCone. #22231

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 22, 2024

This change is Reviewable

@hongkai-dai hongkai-dai force-pushed the mosek_2x2_psd branch 2 times, most recently from 00af5f9 to affd10d Compare November 26, 2024 18:49
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, missing label for release notes (waiting on @hongkai-dai)

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 6 of 6 files at r1, all commit messages.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee AlexandreAmice, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @hongkai-dai)


solvers/test/non_convex_optimization_util_test.cc line 74 at r1 (raw file):

      CompareMatrices(Q1, Q1_expected, 3E-5, MatrixCompareType::absolute));
  EXPECT_TRUE(
      CompareMatrices(Q2, Q2_expected, 3E-5, MatrixCompareType::absolute));

This change seems unrelated.

Code quote:

  EXPECT_TRUE(
      CompareMatrices(Q1, Q1_expected, 3E-5, MatrixCompareType::absolute));
  EXPECT_TRUE(
      CompareMatrices(Q2, Q2_expected, 3E-5, MatrixCompareType::absolute));

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

        continue;
      }
    }

Won't this cause you to skip all the other psd constraints?

Could you add a comment that PositiveSemidefiniteConstraint is only treated as an affine expression in a cone if it is a 2x2 matrix. The other dual solutions are treated in SetVariableBoundsDualSolution

Code quote:

    if constexpr (std::is_same_v<C, PositiveSemidefiniteConstraint>) {
      if (binding.evaluator()->matrix_rows() != 2) {
        continue;
      }
    }

solvers/test/mosek_solver_test.cc line 539 at r1 (raw file):

  MosekSolver solver;
  if (solver.available()) {
    TestTrivial2x2SDP(solver, 1E-5, /*check_dual=*/true, /*dual_tol=*/1E-8);

This change makes the psd constraint less accurate? That seems very odd to me.


solvers/test/mosek_solver_internal_test.cc line 191 at r1 (raw file):

  EXPECT_FALSE(F_subi.empty());
  EXPECT_FALSE(F_subj.empty());
  EXPECT_FALSE(F_valij.empty());

I think that this no longer tests what we want it to test anymore. I would completely remove the 2x2 matrix from this test so that these can stay EXPECT_TRUE

Code quote:

  EXPECT_FALSE(F_subi.empty());
  EXPECT_FALSE(F_subj.empty());
  EXPECT_FALSE(F_valij.empty());

solvers/test/mosek_solver_internal_test.cc line 205 at r1 (raw file):

  auto y = prog.NewContinuousVariables<4>();
  auto X1 = prog.NewSymmetricContinuousVariables<2>();
  auto X2 = prog.NewSymmetricContinuousVariables<3>();

I think here we likely want to add an X3 that is constrained to be in the PSD cone of size 3 or 4. Otherwise this test no longer tests the same thing.

Code quote:

auto X2 = prog.NewSymmetricContinuousVariables<3>();

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