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

Move the warning on small-size PSD matrix to each solver backend. #22136

Conversation

hongkai-dai
Copy link
Contributor

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

Since I am going to change each solver (Mosek, SCS, Clarabel) such that they handle the special-case small-size PSD matrix constraint, I move the warning to each solver. So in the future when the specific solver (like MosekSolver) can handle these small-size PSD constraint, I will only remove the warning for that single solver.


This change is Reviewable

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.

Does Csdp go through the aggregate_constraints code path? If not I think you are missing the warn for that solver.

Reviewed 4 of 4 files at r1, all commit messages.
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 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.

I don't think we should warn for CSDP. CSDP doesn't have a special path for scalar bounding box constraint or second order cone constraint. Instead it just treat them as small-size PSD constraint.

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.

:lgtm:

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @hongkai-dai)

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.

+@sherm1 for platform review please, thanks!

Reviewable status: LGTM missing from assignee sherm1(platform), missing label for release notes (waiting on @hongkai-dai)

@hongkai-dai hongkai-dai added the release notes: fix This pull request contains fixes (no new features) label Nov 6, 2024
Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Platform :lgtm: with a few nits

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions


solvers/constraint.cc line 822 at r1 (raw file):

  } else if (matrix_rows_ == 2) {
    drake::log()->warn(
        "PositiveSemidefiniteConstraint: rows==2, please consider to "

nit: to


solvers/constraint.cc line 896 at r1 (raw file):

        "consider "
        "reformulating this as a rotated Lorentz cone constraint for better "
        "speed/numerics.");

nit: reflow these two messages to reduce the blank space

@hongkai-dai hongkai-dai force-pushed the move_sdp_size_warning_to_solver branch from 866d3ea to 5815f0d Compare November 7, 2024 00:42
@hongkai-dai hongkai-dai merged commit 32ad5f5 into RobotLocomotion:master Nov 7, 2024
9 checks passed
cohnt added a commit to cohnt/drake that referenced this pull request Nov 7, 2024
Merge remote-tracking branch 'upstream/master' into gcs-preprocessing-parallel

[tools] Suppress more Valgrind false positives in MOSEK (RobotLocomotion#22140)

Update parsing infrastructure (RobotLocomotion#22102)

1. MultibodyPlant::RegisterVisualGeometry() can now take a GeometryInstance.
  - Clean up documentation.
  - Extend tests to include what is *actually* done.
2. SDF parser makes use of new API.
   - Stop returning optional<unique_ptr<>> (simply return the unique ptr).
   - Instead of constructing, deconstructing, and reconstructing geometry
     instances, we just pass the instance.
[schema] Add Rotation::Sample (RobotLocomotion#22113)

This bring it on par with Transform::Sample.
Move the warning on small-size PSD matrix to each solver backend. (RobotLocomotion#22136)

[solvers] Fix Gurobi console logging config to happen first (RobotLocomotion#22134)

[bindings] Re-enable some gym test cases on macOS (RobotLocomotion#22112)

[drake_gym] Add info_handler callback (RobotLocomotion#21900)

Set gradient sparsity pattern in LorentzConeConstraint and RotatedLorentzConeConstraint. (RobotLocomotion#22125)

Merge master.

Drop a TODO.

Fix bug when inadvertantly trying to preprocess edges into the start and goal vertex.

Hold a limited number of programs in memory at one time.

Move the construction of the preprocessing program to a separate function for better portability.

Merge up-to-date version of SolveInParallel.

Merge in preprocessing solver options behavior from branch.

wip

lint

Use the preprocessing solver options even if a specific preprocessing solver is not specified.

Fix another segfault.

Bring back the parallelization, revealing the segfault.

Update this test to reflect the new error message.

First pass at parallelizing PreprocessShortestPath.

Fix a segfault when initial_guesses is unset.

Temporarily disable the parallelism for debugging.

finalize interface

wip
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: fix This pull request contains fixes (no new features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants